Skip to content

Conversation

@CollinHowland
Copy link

Work in progress - looking for feedback

Addresses #12245

  • Updating kfp package to use pyproject.toml - depending on feedback, will update other packages accordingly
    • Currently using setuptools as backend but this can be changed in future
    • Removed requirements.in and just placed directly in pyproject.toml to reduce directory clutter
    • We could remove the requirements-dev.txt and requirements-deprecated.txt files if desired and make the dev and deprecated just groups in the pyproject.toml. Example of dev deps as group in pyproject.toml. Lmk your preference.
  • Updated yapf url and version as the existing version doesn't support python 3.13 and was unable to pass pre-commit
    • I would personally recommend moving everything to ruff for the speed and ease but that can be a follow up PR as that can replace many of the existing pre-commit hooks

Regarding the pyproject.toml hierarchy, from my limited research I believe there's a way to define a parent pyproject.toml and then tell it to recognize sub pyproject.toml files. This looked more useful for applying things like common settings (e.g. uv or ruff configurations). I don't think it's worth adding in this PR so far, as I haven't come across a common setting or group of settings to distribute among all pyproject.toml files. But if you do all add uv, ruff, or some other tool that is configured in a pyproject.toml, I recommend creating the parent pyproject.toml accordingly.

Checklist:

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign droctothorpe for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow
Copy link

Hi @CollinHowland. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@github-actions
Copy link

🚫 This command cannot be processed. Only organization members or owners can use the commands.

@CollinHowland CollinHowland marked this pull request as draft September 23, 2025 18:03
@CollinHowland
Copy link
Author

@zazulam take a look and lmk what direction you would like to take for the various dependency types (e.g. necessary dependencies, dev, deprecated) and I can update the pyproject.toml + remove any .txt files as necessary.

Once the desired standard is established, I'll apply it to the rest of the packages mentioned in #12245

@zazulam
Copy link
Collaborator

zazulam commented Sep 24, 2025

@zazulam take a look and lmk what direction you would like to take for the various dependency types (e.g. necessary dependencies, dev, deprecated) and I can update the pyproject.toml + remove any .txt files as necessary.

Once the desired standard is established, I'll apply it to the rest of the packages mentioned in #12245

@CollinHowland I would say to first try setting up the multiple pyproject.tomls, I believe that you might encounter something where the root will need an overarching toml file that would be considered the main "project". As for the various types of deps, they way you currently have them makes sense.

Copy link
Author

@CollinHowland CollinHowland left a comment

Choose a reason for hiding this comment

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

Right now, the requirements txt and in files are still present. Looks like there's a lot of tests and other things that refer to them. I could try updating all of them to refer to the pyproject.toml/packages and not the requirements files, but that could make this PR massive.

LMK @zazulam if you'd like that to be added to this PR, a follow-up PR, or a separate PR that will first be merged into this branch before merging into the upstream master.


[project.optional-dependencies]
# TODO: needs to be dynamically set based on KFP version
# kubernetes = [f'kfp-kubernetes==']
Copy link
Author

Choose a reason for hiding this comment

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

Notably this won't be as easy to get dynamically. I can try dynamically generating a requirements.txt file and grabbing the version. Or some other external thing but because we're not using python like setup.py does, it becomes much more difficult.

LMK if you'd like me to get that dynamic aspect integrated or if there's something we can set (e.g. max, min, or pin)

@zazulam
Copy link
Collaborator

zazulam commented Oct 20, 2025

/ok-to-test

@github-actions
Copy link

Approvals successfully granted for pending runs.

@CollinHowland CollinHowland force-pushed the pyproject branch 2 times, most recently from a612828 to 65597eb Compare October 22, 2025 20:31
Copy link
Author

@CollinHowland CollinHowland left a comment

Choose a reason for hiding this comment

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

Removed the License classifier as it looks like pyproject.toml expects a different format and doesn't want these.

I also made symlinks to the corresponding README's and LICENSE files if they were in some sort of parent directory because the relative path finding when parsing the pyproject.toml doesn't work (e.g. readme = '../../README' can't be found as a valid path)

[project.optional-dependencies]
# TODO: needs to be dynamically set based on KFP version
# kubernetes = [f'kfp-kubernetes==']
kubernetes = ['kfp-kubernetes']
Copy link
Author

Choose a reason for hiding this comment

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

Due to the dynamic version needed, I made this not pinned or less than any specific version.

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