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 component compilation via component decorator #7554

Merged
merged 4 commits into from
Jun 17, 2022
Merged

feat(sdk): enable component compilation via component decorator #7554

merged 4 commits into from
Jun 17, 2022

Conversation

connor-mccarthy
Copy link
Member

@connor-mccarthy connor-mccarthy commented Apr 13, 2022 •
edited
Loading

Description of your changes:

Preserves old component compilation path via @dsl.component decorator for backward compatibility, but raises DeprecationWarning.

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
Copy link
Member Author

/test all

@connor-mccarthy connor-mccarthy marked this pull request as ready for review April 13, 2022 21:41
@connor-mccarthy connor-mccarthy marked this pull request as draft April 19, 2022 17:57
ghost
ghost reviewed Apr 26, 2022
View reviewed changes
sdk/python/kfp/components/component_decorator.py Outdated
raise Exception("output_component_file is not supported yet in v2 early"
"releases and will be added back for v2.0.0 ")
warnings.warn(
"output_component_file parameter is deprecated is deprecated and will eventually be removed. Please use Compiler().compile instead.",
Copy link

Choose a reason for hiding this comment

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

Just noticed you have "is deprecated" written twice - here and in your test :)

@connor-mccarthy
Copy link
Member Author

/hold Merge after #7580.

@connor-mccarthy connor-mccarthy force-pushed the enable-component-compilation-via-component-decorator branch from 82df1aa to f52a3b3 Compare April 29, 2022 15:36
@google-oss-prow google-oss-prow bot added size/XXL and removed size/M labels Apr 29, 2022
@connor-mccarthy
Copy link
Member Author

/test all

@connor-mccarthy connor-mccarthy force-pushed the enable-component-compilation-via-component-decorator branch from d89c7ca to e77c63d Compare May 2, 2022 21:53
@connor-mccarthy connor-mccarthy marked this pull request as ready for review May 2, 2022 21:54
@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy connor-mccarthy marked this pull request as draft May 10, 2022 19:44
@connor-mccarthy connor-mccarthy force-pushed the enable-component-compilation-via-component-decorator branch from e77c63d to 1be8ea8 Compare May 10, 2022 19:54
@connor-mccarthy connor-mccarthy force-pushed the enable-component-compilation-via-component-decorator branch from 1be8ea8 to b859706 Compare May 24, 2022 22:35
@google-oss-prow google-oss-prow bot added size/M and removed size/XXL labels May 24, 2022
@connor-mccarthy
Copy link
Member Author

/unhold

@google-oss-prow google-oss-prow bot removed the lgtm label May 26, 2022
@connor-mccarthy
Copy link
Member Author

/lgtm

Thanks! Do we want to add an release note entry for this PR? Up to you.

Thanks, @chensun. I think we should. I added one.

Can you re-LGTM if all looks good to you?

@connor-mccarthy
Copy link
Member Author

/retest

7 similar comments
@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

2 similar comments
@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

chensun
chensun reviewed Jun 16, 2022
View reviewed changes
sdk/python/kfp/components/component_decorator_test.py Outdated
DeprecationWarning,
r'output_component_file parameter is deprecated and will eventually be removed\.'
):
comp = component(func, output_component_file=filepath)
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 use the decorator syntax instead? since that's what users typically use.

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, we probably ought to. I took this approach to avoid having to redefine the decorated function in each test case, but the boilerplate is probably worth it.

@connor-mccarthy connor-mccarthy requested review from chensun and removed request for ji-yaqi June 16, 2022 19:09
@connor-mccarthy
Copy link
Member Author

@chensun, PTAL

@connor-mccarthy
Copy link
Member Author

/retest

4 similar comments
@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@connor-mccarthy
Copy link
Member Author

/retest

@chensun
Copy link
Member

chensun commented Jun 16, 2022

/lgtm
/approve

Thanks!

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

@connor-mccarthy
Copy link
Member Author

/retest

@google-oss-prow google-oss-prow bot merged commit 6935a47 into kubeflow:master Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

@chensun chensun Awaiting requested review from chensun

Assignees

@chensun chensun

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

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