Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(sdk): enable dynamic importer metadata #7660

Merged
merged 13 commits into from
Sep 19, 2022
Merged

feat(sdk): enable dynamic importer metadata #7660

merged 13 commits into from
Sep 19, 2022

Conversation

connor-mccarthy
Copy link
Member

Description of your changes:
Enables dynamic metadata (passed from pipeline parameter and other inputs) for importer.

Checklist:

@connor-mccarthy connor-mccarthy requested review from chensun and ji-yaqi May 4, 2022 21:01
@google-oss-prow
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@connor-mccarthy
Copy link
Member Author

/test all

@connor-mccarthy
Copy link
Member Author

connor-mccarthy commented May 4, 2022 •
edited
Loading

/hold

The backend does not yet support the $.inputs.parameters['key_name'] syntax yet for importer metadata.

@connor-mccarthy connor-mccarthy force-pushed the fix-importer-metadata-not-supporting-pipeline-parameter branch from a41998f to a7e839d Compare May 4, 2022 21:27
@connor-mccarthy connor-mccarthy force-pushed the fix-importer-metadata-not-supporting-pipeline-parameter branch from 5bcf947 to 87bddc0 Compare August 10, 2022 17:25
@connor-mccarthy
Copy link
Member Author

/test-all

@connor-mccarthy connor-mccarthy force-pushed the fix-importer-metadata-not-supporting-pipeline-parameter branch from 87bddc0 to d29e2cf Compare August 10, 2022 17:26
@connor-mccarthy
Copy link
Member Author

/test-all

@connor-mccarthy
Copy link
Member Author

/test all

@connor-mccarthy connor-mccarthy force-pushed the fix-importer-metadata-not-supporting-pipeline-parameter branch from d29e2cf to 851a50d Compare August 31, 2022 16:36
@connor-mccarthy connor-mccarthy mentioned this pull request Aug 31, 2022
Merged
1 task
@connor-mccarthy connor-mccarthy force-pushed the fix-importer-metadata-not-supporting-pipeline-parameter branch from 851a50d to 75cc74d Compare September 12, 2022 23:14
@connor-mccarthy connor-mccarthy marked this pull request as ready for review September 14, 2022 23:31
@google-oss-prow google-oss-prow bot requested review from IronPan, james-jwu and zijianjoy and removed request for ji-yaqi September 14, 2022 23:32
@connor-mccarthy
Copy link
Member Author

/unhold

chensun
chensun reviewed Sep 19, 2022
View reviewed changes
sdk/python/kfp/components/importer_node.py Outdated Show resolved Hide resolved
sdk/python/kfp/components/importer_node.py
elif isinstance(d, list):
return [traverse_dict_and_create_metadata_inputs(el) for el in d]
else:
return d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR, as I still don't quite follow the practical usage of an importer node when pipeline accepts artifact inputs. But I think there's a bug here in case isinstance(d, str), users may embed a PipelineChannel into a string, e.g.: f'{task1.output}-{name}'. and I think we are missing that parse-and-find logic here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR, as I still don't quite follow the practical usage of an importer node when pipeline accepts artifact inputs.

Agreed -- let's discuss offline about where we can raise this consideration.

I think we are missing that parse-and-find logic here

Updated

@google-oss-prow google-oss-prow bot added size/XL and removed size/L labels Sep 19, 2022
@connor-mccarthy connor-mccarthy force-pushed the fix-importer-metadata-not-supporting-pipeline-parameter branch from 69c265e to 6cd08e8 Compare September 19, 2022 19:58
@connor-mccarthy
Copy link
Member Author

/retest

@google-oss-prow
Copy link

google-oss-prow bot commented Sep 19, 2022 •
edited
Loading

@connor-mccarthy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kubeflow-pipelines-samples-v2 851a50d link true /test kubeflow-pipelines-samples-v2

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.

chensun
chensun reviewed Sep 19, 2022
View reviewed changes
Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Thanks!

@google-oss-prow google-oss-prow bot added the lgtm label Sep 19, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit c353245 into kubeflow:master Sep 19, 2022
@zijianjoy zijianjoy mentioned this pull request Nov 1, 2022
Closed
12 tasks
jlyaoyuli pushed a commit to jlyaoyuli/pipelines that referenced this pull request Jan 5, 2023
@connor-mccarthy @jlyaoyuli
feat(sdk): enable dynamic importer metadata (kubeflow#7660)
100d0f7 
* support placeholder in metadata dict

* add unit tests

* add compilation test

* fix tests

* update compiler test

* use 'metadata' as input key

* update compiler test and golden snapshot

* add int input to compiler test

* change constant name

* use util function

* support f-strings in dynamic import metadata; add and update tests

* update method name after rebase

* make compile consistent across python versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chensun chensun chensun left review comments

@IronPan IronPan Awaiting requested review from IronPan

@james-jwu james-jwu Awaiting requested review from james-jwu

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

Assignees

@chensun chensun

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@connor-mccarthy @chensun