Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-2.fc40.src.rpm This is a minimal, cpu only, no frills or features PyTorch. Reproducible: Always
Taking this review
Copr build: https://copr.fedorainfracloud.org/coprs/build/6511531 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06511531-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Notes from a first pass: - URL should be https://pytorch.org/, define %forgeurl or something like that for the github repo - please add a comment for each patch documenting what it does, and ideally a link to an upstream submission for it, or a note that it's internal only to Fedora and why - why are we statically linking blas? - you can drop the %{?python_provide:%python_provide python3-%{pypi_name}} it's not needed anymore - please use the pyproject macros instead of the deprecated python build macros; this should also allow you to drop all the Requires for the subpackages as they'll be autogenerated - use %{_includedir} instead of hardcoding /usr/include in %prep; also consider symlinking instead of coyping to make it more explicit where these come from - the thirdy_party stuff we're not removing needs to be declared as Provides: bundled() and taken into account in the License field - you shouldn't need to export CC and export CXX, %set_build_flags is called by default these days and does that for you
Additional notes: > %license LICENSE > %license third_party/miniz-2.1.0/LICENSE I'm not sure if this means that you have two license files or not. Double check you're not just overwriting one with another. > %{python3_sitearch}/ This is not allowed, as it's too greedy. Please list out more so that it's more obvious what's in it.
Note that if you switch to pyproject macros to build this, then the %python3_sitelib line disappears entirely from the file list, as those macros generate file list files for you.
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-3.fc40.src.rpm Url changes patches comments, 1 upstreamed static linking of blas is a bug in lapack see BZ 2243823 pyproject-ing fails because this is a cmake project _includedir change made bunding done cc,cxx removed python3_sitearch impoved a bit, this is a very deep path. fixed so versioning
(In reply to Tom Rix from comment #6) > pyproject-ing fails because this is a cmake project It’s not quite that simple. If you can build with %py3_build it’s almost guaranteed that %pyproject_wheel can work, possibly with some minor adjustments. (I maintain several packages that use pyproject-rpm-macros and include a Python extension built with cmake or meson.) I’d like to offer a proof of concept for pytorch, but it may be a few days before I can try it out.
Created attachment 1993806 [details] The .spec file difference from Copr build 6511531 to 6524145
Copr build: https://copr.fedorainfracloud.org/coprs/build/6524145 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06524145-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
(In reply to Ben Beasley from comment #7) > (In reply to Tom Rix from comment #6) > > pyproject-ing fails because this is a cmake project > > It’s not quite that simple. If you can build with %py3_build it’s almost > guaranteed that %pyproject_wheel can work, possibly with some minor > adjustments. (I maintain several packages that use pyproject-rpm-macros and > include a Python extension built with cmake or meson.) I’d like to offer a > proof of concept for pytorch, but it may be a few days before I can try it > out. Please go ahead and try, the upstream location of this spec is https://github.com/trixirt/pytorch-fedora/blob/main/python-torch.spec When your proof of concept plans out, please send me a PR. The autogenerated build requires says we are missing python-cmake and python-ninja. Are these packages in the works ? These missing packages, the docs saying cmake is a WIP and %project_wheel failing lead me to go back to py3_build.
(In reply to Tom Rix from comment #10) > The autogenerated build requires says we are missing python-cmake and > python-ninja. > Are these packages in the works ? Both https://pypi.org/project/cmake/ and https://pypi.org/project/ninja/ are giant hacks to shove the corresponding executable inside binary wheels so that they can be installed in virtualenvs using pip rather than relying on OS-level installations. They don’t include importable Python modules. Since OS-level installations are what we do best, the reasonable thing to do is to patch those dependencies out (noting in a comment that the patch is downstream-only because the dependencies do make sense for the way upstream builds wheels), and just have: BuildRequires: cmake BuildRequires: ninja
(In reply to Ben Beasley from comment #11) > (In reply to Tom Rix from comment #10) > > The autogenerated build requires says we are missing python-cmake and > > python-ninja. > > Are these packages in the works ? > > Both https://pypi.org/project/cmake/ and https://pypi.org/project/ninja/ are > giant hacks to shove the corresponding executable inside binary wheels so > that they can be installed in virtualenvs using pip rather than relying on > OS-level installations. They don’t include importable Python modules. Since > OS-level installations are what we do best, the reasonable thing to do is to > patch those dependencies out (noting in a comment that the patch is > downstream-only because the dependencies do make sense for the way upstream > builds wheels), and just have: > > BuildRequires: cmake > BuildRequires: ninja :) ok we have the same opinion of the python-cmake,ninja I gave it a look when i was trying the pyproject out the first time and went down a rabbit hole
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-4.fc40.src.rpm This brings in the the gloo and xnnpack packages that were recently packaged A --with pyproject option added to make testing the pyproject_ macros easier
Created attachment 1994231 [details] The .spec file difference from Copr build 6524145 to 6532598
Copr build: https://copr.fedorainfracloud.org/coprs/build/6532598 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06532598-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
I believe all of the outstanding comments other than broken pyproject have been addressed. I will be out next week and would like to address any new comments before then. Are there any ?
Just drop all the pyproject stuff since it doesn't work. It's perfectly fine to use the regular python macros instead.
Can you file a ticket against https://src.fedoraproject.org/rpms/pyproject-rpm-macros for the pyproject issue? > License: BSD-3-Clause and MIT This isn't a valid SPDX expression, you need to capitalize the AND > # Broken, looking for cmake,ninja > # %generate_buildrequires > # %pyproject_buildrequires You can't comment out macros, they'll still get expanded and potentially break stuff. Use something like # %%generate_build_requires to escape them or drop these entirely
(In reply to Davide Cavalca from comment #18) > Can you file a ticket against > https://src.fedoraproject.org/rpms/pyproject-rpm-macros for the pyproject > issue? > > > License: BSD-3-Clause and MIT > > This isn't a valid SPDX expression, you need to capitalize the AND > Actually, it's not required. I do need to bug the SPDX people about this, actually. :/ > > # Broken, looking for cmake,ninja > > # %generate_buildrequires > > # %pyproject_buildrequires > > You can't comment out macros, they'll still get expanded and potentially > break stuff. Use something like # %%generate_build_requires to escape them > or drop these entirely Right, that's why I asked for it to be dropped.
SPEC URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-5.fc40.src.rpm dropped the %generate_* capitalized AND Filed a bz 2244862 on pyproject-rpm-macros Left the --with pyproject logic as-is so it could be used as a reproducer in the pyproject-rpm-macros bz
What does "Broken, looking for cmake,ninja" mean?
Upstream says they need the following Python packages to build: https://github.com/pytorch/pytorch/blob/v2.1.0/pyproject.toml [build-system] requires = [ "setuptools", "wheel", "astunparse", "numpy", "ninja", "pyyaml", "cmake", "typing-extensions", "requests", ] Apparently, they are using https://pypi.org/project/cmake/ and https://pypi.org/project/ninja/ -- if you want to use "normal" cmake and ninja instead, unfortunately, you need to patch/sed those out.
I tried running fedora-review against it but there are too many warnings/errors to paste it here, so I'll attach it instead. Things of note: > Header files in -devel subpackage, if present. > Note: python3-torch : /usr/lib64/python3.12/site- > packages/torch/_inductor/codegen/cpp_prefix.h python3-torch : and so on (there's a lot of these) -- are these useful/appropriate to ship it in the main package, or should they be moved to a -devel package? > python3-torch.aarch64: E: zero-length /usr/lib64/python3.12/site-packages/torch/ao/quantization/backend_config/observation_type.py > python3-torch.aarch64: E: zero-length /usr/lib64/python3.12/site-packages/torch/cuda/error.py > python3-torch.aarch64: E: zero-length /usr/lib64/python3.12/site-packages/torch/include/ATen/cudnn/Exceptions.h These need double checking if they're actually supposed to be empty > python3-torch.aarch64: E: unused-direct-shlib-dependency /usr/lib64/python3.12/site-packages/torch/lib/libshm.so /usr/lib64/python3.12/site-packages/torch/lib/libtorch.so.2.1 and so on (there's a lot of these too), see https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency > python3-torch.aarch64: E: undefined-non-weak-symbol /usr/lib64/python3.12/site-packages/torch/lib/libshm.so _gfortran_stop_string (/usr/lib64/python3.12/site-packages/torch/lib/libtorch_cpu.so.2.1) and so on (there's a lot of these too), I'm actually not sure what this one means > python3-torch.aarch64: W: position-independent-executable-suggested /usr/lib64/python3.12/site-packages/torch/bin/torch_shm_manager Generally our build flags should be doing -fPIE/-fPIC by default, are they not getting passed properly? > python3-torch.aarch64: E: non-executable-script /usr/lib64/python3.12/site-packages/torch/_appdirs.py 644 /usr/bin/env python3 and so on (there's a lot of these too), these usually mean you need to strip the shebang from the files as they're not meant to be run directly.
Created attachment 1994491 [details] fedora-review template output
Created attachment 1994495 [details] The .spec file difference from Copr build 6532598 to 6544163
Copr build: https://copr.fedorainfracloud.org/coprs/build/6544163 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06544163-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-6.fc40.src.rpm Clears up the errors. Still several warnings.
Created attachment 1995205 [details] The .spec file difference from Copr build 6544163 to 6557867
Copr build: https://copr.fedorainfracloud.org/coprs/build/6557867 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06557867-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-7.fc40.src.rpm The biggest change was splitting the large lists of files and dirs out to files that are included. Most other errors / warnings cleaned up. The last (current) big error set is Complain about unused lib python3-torch.x86_64: E: unused-direct-shlib-dependency /usr/lib64/python3.12/site-packages/torch/lib/libtorch_python.so.2.1.0 /lib64/libgfortran.so.5 Complain later about undefined symbol that the earlier complained about lib contains python3-torch.x86_64: E: undefined-non-weak-symbol /usr/lib64/python3.12/site-packages/torch/lib/libshm.so.2.1.0 _gfortran_stop_string (/usr/lib64/python3.12/site-\ packages/torch/lib/libtorch_cpu.so.2.1) And when used to both build another package, python-torchvision, and run some samples there is no runtime 'undefined symbol' or any error. This looks like a false positive
Created attachment 1998101 [details] The .spec file difference from Copr build 6557867 to 6615837
Copr build: https://copr.fedorainfracloud.org/coprs/build/6615837 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06615837-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
A minor thing that caught my eye: the title of the review request should be of the form 'Review Request: <main package name here> - <short summary here>'. The '<short summary here>' is usually the %{summary} from the spec file. Looking at the spec file, that would be "An AI/ML python package". However, on PyPI I see "Tensors and Dynamic neural networks in Python with strong GPU acceleration". That's probably too long for the spec file summary, yet more to the point, I guess (except for the GPU acceleration part). Just a shout from the side line. Feel free to ignore.
(In reply to Sandro from comment #33) > A minor thing that caught my eye: the title of the review request should be > of the form 'Review Request: <main package name here> - <short summary > here>'. The '<short summary here>' is usually the %{summary} from the spec > file. Looking at the spec file, that would be "An AI/ML python package". > However, on PyPI I see "Tensors and Dynamic neural networks in Python with > strong GPU acceleration". That's probably too long for the spec file > summary, yet more to the point, I guess (except for the GPU acceleration > part). > > Just a shout from the side line. Feel free to ignore. PyTorch is such a big and well know project, I think of more as a brand name like 'Coke' , that's why i added the '- PyTorch' to this review instead of the short summary in the spec, which as you say isn't the long thing in the git project because it is long and at this point misleading wrt GPU acceleration. CUDA is impossible. ROCm is planned. Sycl/OneAPI would be nice, but at this time unplanned. Vulkan and OpenCL I do not think have full support in PyTorch, missing some/most of the necessary math libs.
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-8.fc40.src.rpm Changes - Expand the gfortran patch to cover more libs needing to link to libgfortran # A Fedora libblas.a problem of undefined symbol # libtorch_cpu.so: undefined symbol: _gfortran_stop_string Patch5: 0001-torch-unresolved-syms-need-gfortran.patch - Most of the -7 fedora-review Errors # libtorch_python.so: undefined symbols: Py* Patch7: 0001-python-torch-link-with-python.patch # E: unused-direct-shlib-dependency libshm.so.2.1.0 libtorch.so.2.1 Patch8: 0001-python-torch-remove-ubuntu-specific-linking.patch The only remaining is python3-torch.x86_64: E: shared-library-without-dependency-information /usr/lib64/python3.12/site-packages/torch/lib/libtorch.so.2.1.0 python3-torch.x86_64: E: files-duplicated-waste 183285
Looks like this breaks fedora-review because of the %include usage. > Source1: pt_dirs.txt > Source2: pt_devel_headers.txt > Source3: pt_python.txt Please add comments to document how these were generated (or even better, generate them with a script and run that in %install instead like the systemd package does). > Summary: An AI/ML python package As the comment above mentioned, this needs to match the summary in the review title; if you want it to include PyTorch for visibility, I'd suggest something like "PyTorch AI/ML framework". > # auto reviewer not happy with : BSD-0-Clause AND Khronos Neither of these are valid SPDX names. BSD-0-Clause is most likely 0BSD (https://spdx.org/licenses/0BSD.html). As for Khronos, you should submit that one for review at https://gitlab.com/fedora/legal/fedora-license-data as I don't see it in the list of allowed licenses (https://docs.fedoraproject.org/en-US/legal/allowed-licenses/). > # Limit to these because they are well behaved with clang > ExclusiveArch: x86_64 aarch64 > %global toolchain clang Is this an upstream limitation? We definitely have other packages using clang that build find on all arches. > # And they need to be stripped Why are we manually stripping stuff in %install?
I'm also unable to rebuild this locally from the last src.rpm: ERROR: Exception(python-torch-2.1.0-8.fc40.src.rpm) Config(fedora-rawhide-aarch64) 0 minutes 8 seconds INFO: Results and/or logs in: /var/lib/mock/fedora-rawhide-aarch64/result ERROR: Source RPM is not installable: Updating / installing... python-torch-2.1.0-8.fc40 warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root warning: user trix does not exist - using root warning: group trix does not exist - using root ######################################## error: unpacking of archive failed on file /builddir/build/SOURCES/pytorch-v2.1.0.tar.gz;655d19b2: cpio: read failed - Inappropriate ioctl for device error: /builddir/build/originals/python-torch-2.1.0-8.fc40.src.rpm cannot be installed
(In reply to Davide Cavalca from comment #36) > Looks like this breaks fedora-review because of the %include usage. > > > Source1: pt_dirs.txt > > Source2: pt_devel_headers.txt > > Source3: pt_python.txt > > Please add comments to document how these were generated (or even better, > generate them with a script and run that in %install instead like the > systemd package does). These are only somewhat automated, some judgement was used to include things. This package has a wide and deep directory / file structure. This approach is a less bad option to comment #4 from Neal. I would prefer to use aggressive globbling. How aggressive can I be ? > > > Summary: An AI/ML python package > > As the comment above mentioned, this needs to match the summary in the > review title; if you want it to include PyTorch for visibility, I'd suggest > something like "PyTorch AI/ML framework". Ok. > > > # auto reviewer not happy with : BSD-0-Clause AND Khronos > > Neither of these are valid SPDX names. BSD-0-Clause is most likely 0BSD > (https://spdx.org/licenses/0BSD.html). As for Khronos, you should submit > that one for review at https://gitlab.com/fedora/legal/fedora-license-data > as I don't see it in the list of allowed licenses > (https://docs.fedoraproject.org/en-US/legal/allowed-licenses/). > Thanks for the 0BSD pointer. From the breakdown of the licenses, the Khronos ones are OpenCL headers They are not used so I am rm-ing them in prep. > > # Limit to these because they are well behaved with clang > > ExclusiveArch: x86_64 aarch64 > > %global toolchain clang > > Is this an upstream limitation? We definitely have other packages using > clang that build find on all arches. To cover the other arches in an early attempt here https://github.com/trixirt/pytorch-fedora/commit/e8602044dda5fe806787b0606508f50d304115a7 Ment using gcc. But there are many, many warnings when gcc is used, because I suspect, pytorch is really only built with clang. So instead of chasing problems with gcc, I reverted back to clang. And rawhide's clang changes faster than pytorch's, this choice of arches are imo the most stable for both pythorch and clang. Maybe this is only x86_64 ? I'd like to get aarch64 in as well. > > > # And they need to be stripped > > Why are we manually stripping stuff in %install? Fixed with fiddling with the cmake build type.
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-9.fc40.src.rpm Above changes made from comment 38 Additions : Replaced the %include lists with -f <generated_list> Add a -test subpackage
Created attachment 2001596 [details] The .spec file difference from Copr build 6615837 to 6695821
Copr build: https://copr.fedorainfracloud.org/coprs/build/6695821 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06695821-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Thank you, this is looking much better now. The only remaining rpmlint issues of note are python3-torch.x86_64: E: shared-library-without-dependency-information /usr/lib64/python3.12/site-packages/torch/lib/libtorch.so.2.1.0 python3-torch.x86_64: W: position-independent-executable-suggested /usr/lib64/python3.12/site-packages/torch/bin/torch_shm_manager which seem to imply our cflags aren't being passed properly somewhere in the build. I'd recommend forcing cmake to be more verbose during the build to verify this. There's also a lot of linker warnings such as /usr/bin/ld: warning: LLVM gold plugin: aten/src/ATen/native/cpu/GridSamplerKernel.cpp:663:5: loop not unrolled: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering but they don't seem to be breaking the build.
There is one significant thing you should do: > Requires: python3dist(filelock) > Requires: python3dist(fsspec) > Requires: python3dist(jinja2) > Requires: python3dist(networkx) > Requires: python3dist(setuptools) > Requires: python3dist(sympy) > Requires: python3dist(typing-extensions) This should all be dropped, since the Python dependency generator already takes care of this.
Spec URL: https://trix.fedorapeople.org/python-torch.spec SRPM URL: https://trix.fedorapeople.org/python-torch-2.1.0-10.fc40.src.rpm pie for torch_shm_manager has been addressed. libtorch error is caused by the global change to use as-needed to fix unused-direct-shlib errors. Since libtorch is not a proper library but rather a wrapper/accumlator of other pythorch libraries the as-needed linking is not approriate, so the new change is to turn off as-needed just for libtorch. This resolves the issue but causes unused-direct-shlib error to be reported for libtorch. These new issues i believe are to be expected for a wrapper library. Requires for python removed Rework to use openblas over blas so the -lgfortran and the static blas,lapack libraries could be removed. gold turned off some verbose output flags added.
Created attachment 2002392 [details] The .spec file difference from Copr build 6695821 to 6714617
Copr build: https://copr.fedorainfracloud.org/coprs/build/6714617 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2242971-python-torch/fedora-rawhide-x86_64/06714617-python-torch/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
From my perspective, this is probably as good as it gets. Unless Davide sees something I'm missing, I think we're good to go here.
Thanks again for working on this! I agree with Neal, this is good to go now. Package APPROVED.
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-torch
Davide and Neal Thanks for all the hard work on the review! Now Fedora can start building out the other PyTorch AI packages
A follow-up discussing the consequences of versioning the .so files in /usr/lib64/python3.12/site-packages/torch/lib/: bug 2253018