[WIP] feat(sdk): block using v2 @component decorator in v1 mode. Fixes #5967 #5989
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sdk/python/kfp/compiler/compiler.py
Outdated
| @@ -678,6 +678,11 @@ def _create_dag_templates(self, pipeline, op_transformers=None, op_to_templates_ | |||
| pipeline_name=self._pipeline_name_param, | |||
| pipeline_root=self._pipeline_root_param, | |||
| launcher_image=self._launcher_image) | |||
|
|
|||
| if "Executor executes v2-based Python function components." in op.command[3] and\ | |||
There was a problem hiding this comment.
I tried to find a better solution for this, but there dont seems to be any variable indication when it is v2 '@component', WDYT @chensun?
There was a problem hiding this comment.
Right, this seems to be quite hacky and unreliable.
but there dont seems to be any variable indication when it is v2 '@component'
How about we add such an indicator variable when @component is used? WDYT?
BTW, thanks for picking up this work!
There was a problem hiding this comment.
Yes, that makes sense to me. A possible solution would be to set:
here:
it is a bit unclear for me what the version argument are used to describe today and is adding a google.com/cloud/pipelines/component/v2 is the right usage here. What are your thoughts @chensun?
There was a problem hiding this comment.
The current plan is to have v2 be a hermetic namespace at some point (even if that means duplication of code across v1 and v2). So I don't think this is the right solution. How about, if v2 components are used, the v1 compiler should auto-compile in v2-compatible mode?
There was a problem hiding this comment.
The current plan is to have v2 be a hermetic namespace at some point...
Could you elaborate on this @neuromage? What does this mean?
How about, if v2 components are used, the v1 compiler should auto-compile in v2-compatible mode?
I would prefer to keep the compiler selected by the users, I think it will be unclear for users(especially new users that are more prone to make this misstake) if the compiler are switched based upon the components.
There was a problem hiding this comment.
@neuromage The following are known breaking changes of v2 compatible mode (some are planned to be resolved, but not all):
- [v2compat] cannot override container image as parameters #5834
- [v2compat] pipeline with output name containing space cannot compile #5831
- v2 compiler has data passing limitations #5711
- [Launcher] support non-root containers in v2 compatible mode #5673
- [v2compat] v1 metrics support #6132
- v2 compatible mode will require KFP backend 1.7.0+, but v1 mode does not have any prerequisites to KFP backend
- v2 compatible mode and v1 mode have different perf / scalability characteristics, which we haven't verified in large scale.
- Because there aren't enough usage, it's very likely there are more changes we haven't found/thought of.
The user doesn't need to know v2 requires a different compilation mode.
I think you'll only be able to say this, until most of the above issues are resolved. Do you think that's the direction we will target?
There was a problem hiding this comment.
I think tracking all the breaking changes in one place is very useful, just filed #6133
There was a problem hiding this comment.
@chensun What are your thoughts on this?
I think there might be some challenges to alter compilation mode based on feature usage, given this decorator isn't the only feature not compatible with v1. If we were to have the pattern of changing compilation mode based on usage. There would be more than one cases, and it would make it hard for users to understand what contributed to the compilation mode change.
So I think throwing an error would be okay for now, and we can revisit this later? @neuromage WDYT?
There was a problem hiding this comment.
I'm ok to throw an error when compiling in v1 mode, but can we should ideally have a better way to detect this rather than looking at the docstrings. I think @chensun is planning for @component decorator to return a v2 class called BaseComponent so we can check for this and throw the error more reliably.
|
@NikeNano: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
lifecycle/stale
|
Closing this PR. No activity for more than a year. /close |
stale
bot
removed
the
lifecycle/stale
|
@rimolive: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Description of your changes:
Fixes #5967
Checklist: