Add standard Python source distribution generation to (pre-)release workflow #152098
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152098
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit e51f387 with merge base 5dfd8a9 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @zklaus could you please add pep517 build as additional source code packaging ? Keeping current tar.gz file as is for now. |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
@pytorchbot revert -m "IMO this PR needs to be split into few helper ones, with better test plan" -c weird |
|
@pytorchbot successfully started a revert job. Check the current status here. |
This reverts commit f16053f. Reverted #152098 on behalf of https://github.com/malfet due to IMO this PR needs to be split into few helper ones, with better test plan ([comment](#152098 (comment)))
|
@zklaus your PR has been successfully reverted. |
|
@zklaus what's your test plan for this change? Also, if symlinks are no longer allowed in PyTorch source repo, that requires a wider discussion |
Let's have a further discussion on this one
How do you mean? Before this change, there was the requirements file After this change, there is still a single source of truth, which now is
This PR is not intended to forbid or remove all symlinks. It does change some symlinks, which are invalid in Python source distributions and tries to point out in sufficient detail problem and remedy in the PR description. Apologies if the description is unclear. As for ittapi, it seems that the problem remains, but as it exists only the Rust subfolder of ittapi's source tree, the suggested remedy (as detailed in the PR description), is to not ship the Rust bindings sources for ittapi.
First, let me point out that this adds a new source artifact without changing the current practice, so that all existing workflows should remain functional. I'm sorry this As for testing, I have compared the file listings of the old and new tarballs and I have verified that Pytorch can be built from the source distribution, both with and without unpacking it. Of course there are many ways to configure a Pytorch build and not every combination can be tested manually. Seeing as this PR improves the source quality by avoiding one broken and several invalid links in the tarball without otherwise impacting existing workflows, I think it would be good to integrate. Short-term, I have planned to also add an actual build step to the tarball creation. @malfet, does this address your concerns? |
Generate source tarball with PEP 517 conform build tools instead of the custom routine in place right now.
Closes #150461.
The current procedure for generating the source tarball consists in creation of a source tree by manual copying and pruning of source files.
This PR replaces that with a call to the standard build tool, which works with the build backend to produce an sdist. For that to work correctly, the build backend also needs to be configured. In the case of Pytorch, the backend currently is (the legacy version of) the setuptools backend, the source dist part of which is mostly configured via the
MANIFEST.infile.The resulting source distribution can be used to install directly from source with
pip install ./torch-{version}.tar.gzor to build wheels directly from source withpip wheel ./torch-{version}.tar.gz; both should be considered experimental for now.Issues
sdist name
According to PEP 517, the name of the source distribution file must coincide with the project name, or more precisely, the source distribution of a project that generates
{NAME}-{...}.whlwheels are required to be named{NAME}-{...}.tar.gz. Currently, the source tarball is calledpytorch-{...}.tar.gz, but the generated wheels and python package are calledtorch-{...}.Symbolic Links
The source tree at the moment contains a small number of symbolic links. This has been seen as problematic largely because of lack of support on Windows, but also because of a problem in setuptools. Particularly unfortunate is a circular symlink in the third party
ittapimodule, which can not be resolved by replacing it with a copy.PEP 721 (now integrated in the Source Distribution Format Specification) allows for symbolic links, but only if they don't point outside the destination directory and if they don't contain
../in their target.The list of symbolic links currently is as follows:
.dockerignore.gitignoredocs/requirements.txt../.ci/docker/requirements-docs.txt..in targetfunctorch/docs/source/notebooks../../notebooks/..in target.github/ci_commit_pins/triton.txt../../.ci/docker/ci_commit_pins/triton.txtthird_party/flatbuffers/docs/source/CONTRIBUTING.md../../CONTRIBUTING.md..in targetthird_party/flatbuffers/java/src/test/java/DictionaryLookup../../../../tests/DictionaryLookup..in targetthird_party/flatbuffers/java/src/test/java/MyGame../../../../tests/MyGame..in targetthird_party/flatbuffers/java/src/test/java/NamespaceA../../../../tests/namespace_test/NamespaceA..in targetthird_party/flatbuffers/java/src/test/java/NamespaceC../../../../tests/namespace_test/NamespaceC..in targetthird_party/flatbuffers/java/src/test/java/optional_scalars../../../../tests/optional_scalars..in targetthird_party/flatbuffers/java/src/test/java/union_vector../../../../tests/union_vector..in targetthird_party/flatbuffers/kotlin/benchmark/src/jvmMain/java../../../../java/src/main/java..in targetthird_party/ittapi/rust/ittapi-sys/c-library../../..in targetthird_party/ittapi/rust/ittapi-sys/LICENSES../../LICENSES..in targetthird_party/opentelemetry-cpp/buildscripts/pre-merge-commit./pre-committhird_party/opentelemetry-cpp/third_party/prometheus-cpp/cmake/project-import-cmake/sample_client.cc../../push/tests/integration/sample_client.cc..in targetthird_party/opentelemetry-cpp/third_party/prometheus-cpp/cmake/project-import-cmake/sample_server.cc../../pull/tests/integration/sample_server.cc..in targetthird_party/opentelemetry-cpp/third_party/prometheus-cpp/cmake/project-import-pkgconfig/sample_client.cc../../push/tests/integration/sample_client.cc..in targetthird_party/opentelemetry-cpp/third_party/prometheus-cpp/cmake/project-import-pkgconfig/sample_server.cc../../pull/tests/integration/sample_server.cc..in targetthird_party/XNNPACK/tools/xngenxngen.pyThe introduction of symbolic links inside the
.ci/dockerfolder creates a new problem, however, because Docker'sCOPYcommand does not allow symlinks in this way. We work around that by usingtar chto dereference the symlinks before handing them over todocker build.Nccl
Nccl used to be included as a submodule. However, with #146073 (first released in v2.7.0-rc1), the submodule was removed and replaced with a build time checkout procedure in
tools/build_pytorch_libs.py, which checks out the required version of nccl from the upstream repository based on a commit pin recorded in.ci/docker/ci_commit_pins/nccl-cu{11,12}.txt.This means that a crucial third party dependency is missing from the source distribution and as the
.cifolder is omitted from the source distribution, it is not possible to use the build time download.However, it is possible to use a system provided Nccl using the
USE_SYSTEM_NCCLenvironment variable, which now also is the default for the official Pytorch wheels.Stack from ghstack (oldest at bottom):
Footnotes
These resources can be naturally considered to be part of the docs, so moving the actual files into the place of the current symlinks and replacing them with (unproblematic) symlinks can be said to improve semantics as well. ↩ ↩2
The flatbuffers docs already actually use the original file, not the symlink and in the most recent releases, starting from flatbuffers-25.1.21 the symlink is replaced by the actual file thanks to a documentation overhaul. ↩
These resources are flatbuffers tests for java and kotlin and can be omitted from our sdist. ↩ ↩2 ↩3 ↩4 ↩5 ↩6 ↩7
We don't need to ship the rust bindings for ittapi. ↩ ↩2
These are demonstration examples for how to link to prometheus-cpp using cmake and can be omitted. ↩ ↩2 ↩3 ↩4