-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename container_image to image for improved UX #3094
base: master
Are you sure you want to change the base?
Changes from all commits
ff7b2c5
5f790d6
f5a72f7
0eb643d
f2ddb80
bb6e187
e66743f
896553e
98547e5
c2383ee
9fab95a
0ca8c17
fdf243d
5dd0c84
c616623
d678e10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import importlib | ||
import re | ||
import warnings | ||
from abc import ABC | ||
from dataclasses import dataclass | ||
from typing import Callable, Dict, List, Optional, TypeVar, Union | ||
|
@@ -43,7 +44,7 @@ def __init__( | |
name: str, | ||
task_config: T, | ||
task_type="python-task", | ||
container_image: Optional[Union[str, ImageSpec]] = None, | ||
image: Optional[Union[str, ImageSpec]] = None, | ||
requests: Optional[Resources] = None, | ||
limits: Optional[Resources] = None, | ||
environment: Optional[Dict[str, str]] = None, | ||
|
@@ -59,7 +60,7 @@ def __init__( | |
:param name: unique name for the task, usually the function's module and name. | ||
:param task_config: Configuration object for Task. Should be a unique type for that specific Task. | ||
:param task_type: String task type to be associated with this Task | ||
:param container_image: String FQN for the image. | ||
:param image: String FQN or ImageSpec for the image. | ||
:param requests: custom resource request settings. | ||
:param limits: custom resource limit settings. | ||
:param environment: Environment variables you want the task to have when run. | ||
|
@@ -82,6 +83,7 @@ def __init__( | |
:param accelerator: The accelerator to use for this task. | ||
:param shared_memory: If True, then shared memory will be attached to the container where the size is equal | ||
to the allocated memory. If str, then the shared memory is set to that size. | ||
:param container_image: Deprecated, please use `image` instead. | ||
""" | ||
sec_ctx = None | ||
if secret_requests: | ||
|
@@ -94,7 +96,23 @@ def __init__( | |
kwargs["metadata"] = kwargs["metadata"] if "metadata" in kwargs else TaskMetadata() | ||
kwargs["metadata"].pod_template_name = pod_template_name | ||
|
||
self._container_image = container_image | ||
# Rename the `container_image` parameter to `image` for improved user experience. | ||
# Currently, both `image` and `container_image` are supported to maintain backward compatibility. | ||
# For more details, please refer to https://github.com/flyteorg/flyte/issues/6140. | ||
if image is not None and kwargs.get("container_image") is not None: | ||
raise ValueError( | ||
"Cannot specify both image and container_image. " | ||
"Please use image because container_image is deprecated and will be removed in the future." | ||
) | ||
elif kwargs.get("container_image") is not None: | ||
warnings.warn( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we just make this info? warning is a little too annoying I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this is not annoying enough. If we really want users to change their code, then I think warnings should be a bit annoying. Because when we remove this feature, breaking code will be even more annoying. |
||
"container_image is deprecated and will be removed in the future. Please use image instead.", | ||
DeprecationWarning, | ||
) | ||
self._image = kwargs["container_image"] | ||
eapolinario marked this conversation as resolved.
Show resolved
Hide resolved
|
||
else: | ||
self._image = image | ||
|
||
# TODO(katrogan): Implement resource overrides | ||
self._resources = ResourceSpec( | ||
requests=requests if requests else Resources(), limits=limits if limits else Resources() | ||
|
@@ -138,9 +156,28 @@ def __init__( | |
def task_resolver(self) -> TaskResolverMixin: | ||
return self._task_resolver | ||
|
||
@property | ||
def image(self) -> Optional[Union[str, ImageSpec]]: | ||
return self._image | ||
|
||
@property | ||
def container_image(self) -> Optional[Union[str, ImageSpec]]: | ||
return self._container_image | ||
"""Deprecated, please use `image` instead.""" | ||
return self._image | ||
|
||
@property | ||
def _container_image(self) -> Optional[Union[str, ImageSpec]]: | ||
"""Deprecated, please use `image` instead.""" | ||
return self._image | ||
|
||
@_container_image.setter | ||
def _container_image(self, image: Optional[Union[str, ImageSpec]]): | ||
JiangJiaWei1103 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""Deprecated, please use `image` instead. | ||
|
||
This setter is for backward compatibility, so that setting `_container_image` | ||
will adjust the new `_image` parameter directly. | ||
""" | ||
self._image = image | ||
|
||
@property | ||
def resources(self) -> ResourceSpec: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider enhancing the deprecation notice for
_container_image
by adding a warning using thewarnings
module. This would help users transition to using_image
instead.Code suggestion
Code Review Run #4f136e
Is this a valid issue, or was it incorrectly flagged by the Agent?