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): support input list of artifacts in Custom Container Components [lists of artifacts support pt. 4] #8484

Merged
merged 1 commit into from
Feb 16, 2023
Merged

feat(sdk): support input list of artifacts in Custom Container Components [lists of artifacts support pt. 4] #8484

merged 1 commit into from
Feb 16, 2023

Conversation

connor-mccarthy
Copy link
Member

@connor-mccarthy connor-mccarthy commented Nov 18, 2022 •
edited
Loading

Description of your changes:
Supports using lists of artifacts in Custom Container Components by enabling writing of placeholders of the form "{{$.inputs.artifacts['input_name']}}" via the following syntax:

@dsl.container_component
def comp(input_list: Input[List[Artifact]]):
    return dsl.ContainerSpec(image='alpine', command=['echo'], args=[input_list])

Also, write the isArtifactList field all components types.

Creating output lists of artifacts is not yet supported, but will be supported by #8808. This change will also include type checking logic.

Checklist:

@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 connor-mccarthy force-pushed the list-of-artifacts-placeholders branch from 0b6729e to 67592a9 Compare December 3, 2022 01:17
@connor-mccarthy
Copy link
Member Author

/test all

@connor-mccarthy connor-mccarthy force-pushed the list-of-artifacts-placeholders branch from 67592a9 to 7dc2f55 Compare December 5, 2022 23:12
@connor-mccarthy connor-mccarthy marked this pull request as ready for review December 5, 2022 23:12
@google-oss-prow google-oss-prow bot requested a review from james-jwu December 5, 2022 23:13
connor-mccarthy
connor-mccarthy commented Dec 5, 2022
View reviewed changes
sdk/python/kfp/components/component_factory.py
@@ -467,6 +467,37 @@ def create_component_from_func(
component_spec=component_spec, python_func=func)


def make_input_for_parameterized_container_component_function(
Copy link
Member Author

Choose a reason for hiding this comment

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

This extracts existing code into a function and adds handling for new placeholders into the function

@connor-mccarthy
Copy link
Member Author

/unhold
/assign @chensun

@connor-mccarthy
Copy link
Member Author

/hold until BE supports these placeholders

@connor-mccarthy connor-mccarthy changed the title feat(sdk): support list of artifacts in Custom Container Components [lists of artifacts support pt. 4] feat(sdk): support input list of artifacts in Custom Container Components [lists of artifacts support pt. 4] Jan 3, 2023
@connor-mccarthy connor-mccarthy mentioned this pull request Feb 6, 2023
Merged
1 task
@connor-mccarthy connor-mccarthy force-pushed the list-of-artifacts-placeholders branch 2 times, most recently from 7c3e44a to 0d5533b Compare February 14, 2023 22:09
@connor-mccarthy
Copy link
Member Author

/unhold

chensun
chensun reviewed Feb 14, 2023
View reviewed changes
sdk/python/test_data/components/container_component_with_list_of_artifacts.py Outdated
@dsl.container_component
def comp_with_artifact_list(input_list: Input[List[Artifact]]):
return dsl.ContainerSpec(
image='alpine', command=[input_list], args=[input_list])
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have an example that's executable to show case how the container can consume the list of artifacts, like printing out the artifact names, etc. Without that, the input is an opaque block and it's unclear to users how to actually use list of artifacts in container component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. For this PR, I've opted to remove read/write tests altogether since lists of artifacts are unusable until ParallelFor fan-in is implemented.

I will add real-world test cases in alongside ParallelFor fan-in (#8808).

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

sdk/python/test_data/components/container_component_with_list_of_artifacts.yaml Outdated
comp-comp-with-artifact-list:
executorLabel: exec-comp-with-artifact-list
inputDefinitions:
artifacts:
Copy link
Member

Choose a reason for hiding this comment

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

Should we expect to see is_artifact_list in the YAML?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, updated.

sdk/python/kfp/components/placeholders.py Outdated
def __getattribute__(self, name: str) -> Any:
if name in {'name', 'uri', 'metadata', 'path'}:
raise AttributeError(
f'Cannot access an attribute on a list of artifacts in a Custom Container Component. Found refence to attribute {name!r} on {self.input_name!r}. Please pass the whole list of artifacts only.'
Copy link
Member

Choose a reason for hiding this comment

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

typo: refence

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Updated.

@connor-mccarthy connor-mccarthy force-pushed the list-of-artifacts-placeholders branch from 0d5533b to 6a109dc Compare February 15, 2023 20:51
@google-oss-prow
Copy link

@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-sdk-execution-tests 6a109dc link true /test kubeflow-pipelines-sdk-execution-tests

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 approved these changes Feb 16, 2023
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

@google-oss-prow google-oss-prow bot added the lgtm label Feb 16, 2023
@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

@connor-mccarthy connor-mccarthy merged commit 4175260 into kubeflow:master Feb 16, 2023
jlyaoyuli pushed a commit to jlyaoyuli/pipelines that referenced this pull request Mar 9, 2023
@connor-mccarthy @jlyaoyuli
support list of artifact input placeholders (kubeflow#8484)
2878734
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chensun chensun chensun approved these changes

Assignees

@chensun chensun

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

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