Rename container_image to image for improved UX #3094
Conversation
1. Support both `image` and `container_image` for backward compatibility
2. Modify the core decorator used for any task type
* We focus on the user-facing inteface
Signed-off-by: JiangJiaWei1103 <[email protected]>
Code Review Agent Run Status
|
…tests Signed-off-by: JiangJiaWei1103 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3094 +/- ##
==========================================
- Coverage 82.21% 74.54% -7.68%
==========================================
Files 202 202
Lines 21430 21477 +47
Branches 2760 2768 +8
==========================================
- Hits 17618 16009 -1609
- Misses 3011 4687 +1676
+ Partials 801 781 -20 ☔ View full report in Codecov by Sentry. |
Code Review Agent Run Status
|
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Code Review Agent Run Status
|
JiangJiaWei1103
changed the title
JiangJiaWei1103
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
flytekit/core/task.py
Outdated
| @@ -584,6 +606,24 @@ async def eager_workflow(x: int) -> int: | |||
| async def eager_workflow(x: int) -> int: | |||
| ... | |||
| """ | |||
| # Rename the `container_image` parameter to `image` for improved user experience. | |||
There was a problem hiding this comment.
Should image be added to PythonAutoContainerTask and we do the validation there? This way @eager (through EagerAsyncPythonFunctionTask) and @task will get the check.
For backward compatible, I would have PythonAutoContainerTask._image, but have ._container_image and container_image be a mirror of it:
class PythonAutoContainerTask:
def __init__(self, container_image, image):
# validate image and container_image
self._image = container_image
@property
def container_image(self):
return self._image
@property
def _container_image(self):
return self._image
@_container_image.setter
def _container_image(self, value):
self._image = value
@property
def image(self):
return self._imageThere was a problem hiding this comment.
Both work fine for me. Thanks for your suggestion--I learned a lot!
I have two questions:
- Is it common practice to make a class’s internal attribute (like
_container_imagehere) accessible and modifiable from outside the class?- I assume we do this to avoid breaking users’ code.
- Would it be good to add a
setterforimage?
There was a problem hiding this comment.
I assume we do this to avoid breaking users’ code.
I'm mostly trying to keep backward compatibility. I see bunch of code paths that does obj._container_image = . (I do not really like this practice, but that's the current state of the code base)
Would it be good to add a setter for image?
I'll say it's not worth it here. If we keep the obj._container_image = practice, then future users can do obj._image = .
There was a problem hiding this comment.
Thanks for the clarification! Everything is now perfectly clear.
Code Review Agent Run #e396c5Actionable Suggestions - 3
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| @@ -380,7 +380,7 @@ class DC: | |||
| a: Union[int, bool, str, float] | |||
| b: Union[int, bool, str, float] | |||
| @task(container_image=custom_image) | |||
| @task(image=custom_image) | |||
There was a problem hiding this comment.
Consider updating the parameter name from image to container_image to maintain backward compatibility and avoid potential breaking changes for existing code.
Code suggestion
Check the AI-generated fix before applying
| @task(image=custom_image) | |
| @task(container_image=custom_image) |
Code Review Run #e396c5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -104,6 +105,7 @@ def task( | |||
| interruptible: Optional[bool] = ..., | |||
| deprecated: str = ..., | |||
| timeout: Union[datetime.timedelta, int] = ..., | |||
| image: Optional[Union[str, ImageSpec]] = ..., | |||
There was a problem hiding this comment.
Consider consolidating the image and container_image parameters since they appear to serve the same purpose. Having two parameters for the same functionality could lead to confusion.
Code suggestion
Check the AI-generated fix before applying
- image: Optional[Union[str, ImageSpec]] = ...,
- container_image: Optional[Union[str, ImageSpec]] = ...,
+ container_image: Optional[Union[str, ImageSpec]] = ..., # Use this instead of image parameter
+ image: Optional[Union[str, ImageSpec]] = None, # Deprecated: Use container_image instead
Code Review Run #e396c5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -143,6 +145,7 @@ def task( | |||
| interruptible: Optional[bool] = ..., | |||
| deprecated: str = ..., | |||
| timeout: Union[datetime.timedelta, int] = ..., | |||
| image: Optional[Union[str, ImageSpec]] = ..., | |||
There was a problem hiding this comment.
Consider consolidating the image parameter with the existing container_image parameter to avoid semantic duplication, as both appear to serve the same purpose of specifying container images.
Code suggestion
Check the AI-generated fix before applying
- image: Optional[Union[str, ImageSpec]] = ...,
- container_image: Optional[Union[str, ImageSpec]] = ...,
+ container_image: Optional[Union[str, ImageSpec]] = ..., # Also accepts image parameter for backward compatibility
Code Review Run #e396c5
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
1. Validate `image` and `container_image` in `PythonAutoContainerTask`'s
initializer
2. Maintain backward compatibility within `PythonAutoContainerTask`
Signed-off-by: JiangJiaWei1103 <[email protected]>
Code Review Agent Run #5e8094Actionable Suggestions - 2
Review Details
AI Code Review powered by |
| image=image, | ||
| container_image=container_image, |
There was a problem hiding this comment.
Consider consolidating the image parameters. Currently both image and container_image are being passed, which could lead to confusion about precedence. Since container_image is deprecated, it would be clearer to use only image.
Code suggestion
Check the AI-generated fix before applying
| image=image, | |
| container_image=container_image, | |
| image=image, |
Code Review Run #5e8094
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| """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]]): | ||
| self._image = image |
There was a problem hiding this comment.
Consider consolidating the duplicate property definitions for container_image and _container_image. Both properties return self._image and have the same docstring.
Code suggestion
Check the AI-generated fix before applying
- def container_image(self) -> Optional[Union[str, ImageSpec]]:
- """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]]):
+ @property
+ def _container_image(self) -> Optional[Union[str, ImageSpec]]:
+ """Deprecated, please use `image` instead."""
+ return self._image
Code Review Run #5e8094
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| return self._image | ||
|
|
||
| @_container_image.setter | ||
| def _container_image(self, image: Optional[Union[str, ImageSpec]]): |
There was a problem hiding this comment.
| def _container_image(self, image: Optional[Union[str, ImageSpec]]): | |
| def _container_image(self, image: Optional[Union[str, ImageSpec]]): | |
| """Deprecated, please use `image` instead.""" | |
| # This setter is for backward compatibility, so that setting `_container_image` | |
| # will adjust the new `_image` parameter directly. |
There was a problem hiding this comment.
Hi Thomas,
I’ve just added i. Thanks for your guidance!
Signed-off-by: JiangJiaWei1103 <[email protected]>
JiangJiaWei1103
dismissed stale reviews from thomasjpfan and pingsutw
via
e66743f
Code Review Agent Run #acb303Actionable Suggestions - 0Review Details
AI Code Review powered by |
Tracking issue
Closes flyteorg/flyte#6140.
Why are the changes needed?
To enhance the user experience, the concept of
containershould be abstracted from flytekit users.What changes were proposed in this pull request?
At this early stage, we propose to focus on the user-facing codebase and support both
imageandcontainer_imagefor backward compatibility. There exist three cases:imageandcontainer_imageare specified: Raise an errorcontainer_imageis used: Warn users about the future deprecationimageis used: UseimagedirectlyIn the future, we can revisit whether modifying the developer-facing code is beneficial when it becomes more relevant.
How was this patch tested?
This patch has been tested using modified and newly added unit tests.
Setup process
Run all unit tests using the following command:
Check all the applicable boxes
Related PRs
NA
Docs link
Please refer to flyteorg/flyte#6211
Summary by Bito
This PR implements the renaming of 'container_image' parameter to 'image' across Flytekit codebase, with focus on PythonAutoContainerTask class. The implementation includes deprecation warnings, @Property and @deprecated decorators while maintaining backward compatibility. The changes include centralized warning logic and error handling for simultaneous parameter specification, improving code clarity and user experience.Unit tests added: True
Estimated effort to review (1-5, lower is better): 3