chore(sdk): migrate build system to pyproject.toml (#12245)#12840
chore(sdk): migrate build system to pyproject.toml (#12245)#12840Anchit-04 wants to merge 1 commit intokubeflow:masterfrom
Conversation
Signed-off-by: Anchit <anchit1234deshpande@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🎉 Welcome to the Kubeflow Pipelines repo! 🎉 Thanks for opening your first PR! We're excited to have you onboard 🚀 Next steps:
Feel free to ask questions in the comments. |
|
Hi @Anchit-04. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
There was a problem hiding this comment.
Pull request overview
Migrates the sdk/python (KFP Python SDK) packaging configuration toward a PEP 517/518 + PEP 621 workflow by introducing pyproject.toml with hatchling as the build backend, while attempting to keep setup.py present for legacy workflows.
Changes:
- Added
sdk/python/pyproject.tomlwith project metadata, dependencies, entry points, and hatch versioning. - Updated
sdk/python/setup.pyto add an additionalsetup()invocation under__main__. - Declared optional dependency groups (extras) and console scripts in
pyproject.toml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sdk/python/setup.py | Adds a setup() call intended to support legacy invocation paths. |
| sdk/python/pyproject.toml | Introduces hatchling-based build config and moves metadata/dependencies into PEP 621 format. |
|
|
||
| if __name__ == "__main__": | ||
| setup() |
There was a problem hiding this comment.
setup.py currently invokes setuptools.setup(...) at import time and then calls setup() again under if __name__ == "__main__". When setup.py is executed by build/install tooling, this results in two setup() invocations in one process, which can cause failures or undefined behavior during packaging.
Suggested fix:
- Remove the second
setup()call, or refactor the file into a true shim (i.e., avoid callingsetuptools.setup(...)unconditionally and delegate to the PEP 517 backend instead).
| if __name__ == "__main__": | |
| setup() |
| import setuptools | ||
| from setuptools import setup | ||
|
|
There was a problem hiding this comment.
from setuptools import setup is only used for the new setup() call under __main__. If the intent is to keep setuptools.setup(...) as the single entrypoint (or to turn this file into a shim), this import should be removed to avoid confusion and accidental double-setup behavior.
| "urllib3 < 2.0.0", | ||
| "protobuf >= 3.13.0", |
There was a problem hiding this comment.
The dependency set in pyproject.toml does not match the SDK’s existing published requirements (requirements.in / requirements.txt) that setup.py currently installs. This will make PEP 517 builds (from pyproject.toml) install a materially different dependency set than legacy installs/editables, and is likely to break runtime imports.
Concrete issues in this block:
kfp-pipeline-specandgoogle-api-coreare missing even though the code importskfp.pipeline_spec.*and prior requirements pingoogle-api-core.- Several bounds differ from the established constraints (e.g.,
protobuflowered to>=3.13.0vsrequirements.in’sprotobuf>=6.31.1,<7.0,urllib3<2.0.0vs<3.0.0,requests-toolbeltupper bound,kubernetesupper bound).
Suggested fix:
- Make
pyproject.tomldependencies identical torequirements.in(including environment markers and upper bounds), or adopt a single source of truth (e.g., drive both from one file/tool) to avoid divergence.
| "urllib3 < 2.0.0", | |
| "protobuf >= 3.13.0", | |
| "urllib3 < 3.0.0", | |
| "protobuf >= 6.31.1, < 7.0", | |
| "kfp-pipeline-spec", | |
| "google-api-core", |
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
| kubernetes = ["kfp-kubernetes"] |
There was a problem hiding this comment.
The kubernetes extra previously pinned kfp-kubernetes to the exact SDK version (see setup.py’s kfp-kubernetes=={_version}), which helps keep the extension layer compatible with the installed SDK. In pyproject.toml it is now unpinned ("kfp-kubernetes"), which can lead to installing an incompatible kfp-kubernetes version.
Suggested fix:
- Restore an explicit compatible version constraint for
kfp-kubernetes(ideally aligned to the SDK versioning policy used elsewhere in the repo), rather than leaving it unconstrained.
| kubernetes = ["kfp-kubernetes"] | |
| kubernetes = ["kfp-kubernetes >= 2.0.0, < 3.0.0dev"] |
| ] | ||
| docker = ["docker"] | ||
| all = [ | ||
| "kfp[kubernetes,notebooks,docker]" |
There was a problem hiding this comment.
all = ["kfp[kubernetes,notebooks,docker]"] makes the project’s all extra depend on the same project again with different extras. This self-referential dependency can confuse resolvers and is avoidable; the prior setup.py expands all to the union of the extra dependency lists directly.
Suggested fix:
- Expand
allto directly list the union of dependencies fromkubernetes,notebooks, anddockerinstead of referencingkfp[...].
| "kfp[kubernetes,notebooks,docker]" | |
| "kfp-kubernetes", | |
| "nbclient >= 0.10, < 1", | |
| "ipykernel >= 6, < 7", | |
| "jupyter_client >= 7, < 9", | |
| "docker", |
|
This probably should be synced with #12770. |
I have migrated the Python SDK from the old setup.py to the modern pyproject.toml using hatchling as the build backend. This is part of the modernization task for issue #12245.
List of Changes I have made:
Checklist: