fix(sdk): make component names more unique when merging specs #9969
Conversation
google-oss-prow
bot
added
do-not-merge/work-in-progress
size/XS
needs-ok-to-test
labels
|
Hi @stijntratsaertit. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
stijntratsaertit
changed the title
|
@stijntratsaertit, can you describe what you mean by more unique? Have you encountered a key collision? |
|
Hi, I indeed encountered a key collision, more specifically when dealing with multiple for loops. It seems to go wrong when it's merging the specs. In VertexAI Pipelines, the UI was rendering wrong components within a for loop and in YAML I was able to see that wrong reference because the componentRef of a for-loop was pointing to a different component. The names will now be more unique in the sense that it will now generate a new name when the same name exists in the current subgroup components (this was my initial change). I have now gone back to it and noticed an additional change would be needed. The renaming should be done starting from the last generated name mapping, in order to avoid a case where unintentional renames were being done as well. |
|
/ok-to-test Thanks for explaining, @stijntratsaertit. Could you please add one test cases for each key conflict case you discovered? It should probably go here. |
stijntratsaertit
force-pushed
the
make-component-names-in-dag-more-unique
branch
from
3d1a09d
to
c2c2f16
Compare
|
/retest |
|
@stijntratsaertit, feel free to ignore |
|
|
||
| with dsl.Condition(run_pipeline_3 == run_pipeline_3): | ||
| with dsl.Condition(run_pipeline_3 == 1): | ||
| pipeline_3() |
There was a problem hiding this comment.
Is this a minimal example? If not, could we reduce this to the minimal example required to reproduce the problematic behavior? This will help us ensure we don't regress and will reduce the likelihood that the test is removed as redundant or unclear in the future.
Also, consider renaming the test method to reflect the behavior under test and, if needed, adding a docstring to describe in more detail.
There was a problem hiding this comment.
Should be much better now, thanks for the push
|
/retest |
| item for sublist in component_refs for item in sublist | ||
| ] | ||
| counted_refs = collections.Counter(all_component_refs) | ||
| self.assertEqual(1, max(counted_refs.values())) |
There was a problem hiding this comment.
In my local testing, this passes both before and after the changes. I also observe that the YAML is not different. Is it possible there is something missing from this test case?
There was a problem hiding this comment.
My bad - went to harsh on cutting components to a minimal version.
stijntratsaertit
force-pushed
the
make-component-names-in-dag-more-unique
branch
2 times, most recently
from
8ce988e
to
8f6834f
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
stijntratsaertit
force-pushed
the
make-component-names-in-dag-more-unique
branch
from
8f6834f
to
294403f
Compare
|
[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 |
|
As I pulled master in to update the staleness of the PR, issues will show when attempting to fix the DCO check. What would be the correct approach to resolve this in my case? |
|
/retest |
|
@stijntratsaertit: 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. |
Description of your changes:
When having having multiple levels of nesting in pipelines (especially when using a similar pipeline), components have a greater chance to conflict in naming and resulting in wrong references in the YAML.
Using the following change, names of already renamed components are also taken into account when altering a component name.
Checklist: