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.

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

fix(sdk): fix kfp component build only produces empty requirements.txt #8372

Merged

Conversation

chensun
Copy link
Member

@chensun chensun commented Oct 17, 2022

Description of your changes:

  • Collect runtime dependencies specified via packages_to_install, and install them at container build time.
  • Rename the system generated requirements.txt to runtime-requirements.txt so that users may keep their requirements.txt which may contain dependencies for the entire project.
  • Always update/overwrite runtime-requirements.txt every time users run kfp component build.

Checklist:

Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

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

/lgtm

This looks great. Thanks, @chensun.

A few thoughts that do not need to block merge:

  • Perhaps add a release note, particularly to note the preservation of the user environment requirements.txt file
  • Unrelated to this PR: I think it might be good to update the CLI help text (docstrings) to use language that is consistent with our v2 documentation ("Containerized Python Components") here and here. The lack of identical language makes the current text slightly suggestive of CUJ3, particularly on line component.py:334. We can also probably drop the "v2" in the help text. IMO, this is implied by the kfp version the user is using, but I can see other views on this.

@google-oss-prow google-oss-prow bot added the lgtm label Oct 17, 2022
@chensun
Copy link
Member Author

chensun commented Oct 17, 2022

/lgtm

This looks great. Thanks, @chensun.

A few thoughts that do not need to block merge:

  • Perhaps add a release note, particularly to note the preservation of the user environment requirements.txt file
  • Unrelated to this PR: I think it might be good to update the CLI help text (docstrings) to use language that is consistent with our v2 documentation ("Containerized Python Components") here and here. The lack of identical language makes the current text slightly suggestive of CUJ3, particularly on line component.py:334. We can also probably drop the "v2" in the help text. IMO, this is implied by the kfp version the user is using, but I can see other views on this.

All good points! I prepared a release PR that will add the release note for this and some previous PRs.
For the second bullet point, can you help me with the effort?

@chensun
Copy link
Member Author

chensun commented Oct 17, 2022

/approve

@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 54922b3 into kubeflow:master Oct 17, 2022
@chensun chensun deleted the kfp-component-build-requirements branch October 17, 2022 19:51
jlyaoyuli pushed a commit to jlyaoyuli/pipelines that referenced this pull request Jan 5, 2023
…txt (kubeflow#8372)

* fix CUJ2 always writes empty requirements.txt

* sort dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants