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 <waynechuang97@gmail.com>
Code Review Agent Run Status
|
…tests Signed-off-by: JiangJiaWei1103 <waynechuang97@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3094 +/- ##
==========================================
+ Coverage 80.01% 83.58% +3.57%
==========================================
Files 318 3 -315
Lines 27075 195 -26880
Branches 2779 0 -2779
==========================================
- Hits 21663 163 -21500
+ Misses 4647 32 -4615
+ Partials 765 0 -765 ☔ View full report in Codecov by Sentry. |
Code Review Agent Run Status
|
Signed-off-by: JiangJiaWei1103 <waynechuang97@gmail.com>
Signed-off-by: JiangJiaWei1103 <waynechuang97@gmail.com>
Signed-off-by: JiangJiaWei1103 <waynechuang97@gmail.com>
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.
|
1. Validate `image` and `container_image` in `PythonAutoContainerTask`'s
initializer
2. Maintain backward compatibility within `PythonAutoContainerTask`
Signed-off-by: JiangJiaWei1103 <waynechuang97@gmail.com>
Code Review Agent Run #5e8094Actionable Suggestions - 2
Review Details
AI Code Review powered by |
| 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 it. Thanks for your guidance!
Signed-off-by: JiangJiaWei1103 <waynechuang97@gmail.com>
JiangJiaWei1103
dismissed stale reviews from thomasjpfan and pingsutw
via
e66743f
Code Review Agent Run #acb303Actionable Suggestions - 0Review Details
AI Code Review powered by |
Code Review Agent Run #74c115Actionable Suggestions - 0Additional Suggestions - 10
Review Details
AI Code Review powered by |
|
shoulnd't we also support |
|
Hi @eapolinario, Thanks for the suggestion! We focus on Supporting |
| @@ -57,7 +59,8 @@ 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 for the image. | |||
There was a problem hiding this comment.
| :param image: String FQN for the image. | |
| :param image: String FQN or ImageSpec for the image. |
| @@ -42,6 +43,7 @@ def __init__( | |||
| name: str, | |||
| task_config: T, | |||
| task_type="python-task", | |||
| image: Optional[Union[str, ImageSpec]] = None, | |||
| container_image: Optional[Union[str, ImageSpec]] = None, | |||
There was a problem hiding this comment.
@eapolinario and I were talking about this and we were thinking we might want to put this behind kwargs instead. Could we try that please?
Like delete this line... but leave it in the docs below, maybe move it to the bottom of the params, and check for it explicitly in the code to see if it's set (like kwargs["container_image"]). This way we can hide it from the user, they don't see it in the IDE etc.
cc @thomasjpfan
There was a problem hiding this comment.
put this behind kwargs instead
+1
There was a problem hiding this comment.
It's a bit non-standard, but I am okay with hiding it in kwargs, but keeping it in the docs.
| "Please use image because container_image is deprecated and will be removed in the future." | ||
| ) | ||
| elif container_image is not None: | ||
| warnings.warn( |
There was a problem hiding this comment.
should we just make this info? warning is a little too annoying I think.
There was a problem hiding this comment.
I feel like this is not annoying enough. DeprecationWarning does not really show up with default filter warnings. (FutureWarning would be more annoying)
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.
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 both the renaming of 'container_image' parameter to 'image' across Flytekit codebase and introduces comprehensive error handling improvements. Changes include deprecation warnings, secure handling of pip arguments, improved async operations, and better Kubernetes resource validation. The implementation maintains backward compatibility while adding centralized warning logic and error handling for better runtime stability.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5