[Bug 2259226] New: Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Bug ID: 2259226
           Summary: Review Request: python-chainer - A flexible framework
                    of neural networks
           Product: Fedora
           Version: rawhide
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

Spec URL: https://trix.fedorapeople.org/python-chainer.spec
SRPM URL: https://trix.fedorapeople.org/python-chainer-7.8.1-1.fc40.src.rpm

Chainer is a Python-based deep learning framework aiming at flexibility.        
It provides automatic differentiation APIs based on the define-by-run           
approach (a.k.a. dynamic computational graphs) as well as object-oriented       
high-level APIs to build and train neural networks.

Reproducible: Always

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Tom Rix  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review?
             Blocks|                            |1011110 (ML-SIG)

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1011110
[Bug 1011110] Machine Learning SIG - review tracker

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Tom Rix  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2255637

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=2255637
[Bug 2255637] Review Request: einops - Deep learning operations reinvented

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Fedora Review Service  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://chainer.org

--- Comment #1 from Fedora Review Service  ---
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6922131
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...

Found issues:

- Upstream MD5sum check error, diff is in
/var/lib/copr-rpmbuild/results/python-chainer/diff.txt
  Read more:
https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/

Please know that there can be false-positives.

---
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.

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Ben Beasley  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #2 from Ben Beasley  ---
It looks like the archive from Source0 does not match the one in the SRPM. The
one in the SRPM more closely resembles the PyPI sdist, but is not identical to
that, either.

Which source archive did you intend to use?

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

--- Comment #3 from Tom Rix  ---
Spec URL: https://trix.fedorapeople.org/python-chainer.spec
SRPM URL: https://trix.fedorapeople.org/python-chainer-7.8.1-1.fc40.src.rpm

Sorry about this, source0 fixed up in this one.

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Fedora Review Service  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |AutomationTriaged

--- Comment #4 from Fedora Review Service  ---
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6987342
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...

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.

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Ben Beasley  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|[e-mail]    |[e-mail]
              Flags|fedora-review?              |

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Ben Beasley  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review?

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

--- Comment #5 from Ben Beasley  ---
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

- The AppVeyor files in appveyor/ are both CC0-1.0 and clearly code rather than
  content. Since CC0-1.0 is not-allowed for code in Fedora, I think you will
  need to filter these out of the source archive before uploading it to the
  lookaside cache. (Even though they do not contribute to the licenses of the
  binary RPMs, these files would still be distributed in the source RPMs.)

I know this feels a bit silly, and you’re welcome to get a “second opinion”
  from Fedora Legal if you think it should be unnecessary. There is an
  exception that covers somewhat similar situations,

https://gitlab.com/fedora/legal/fedora-license-data/-/blob/ee033a25064376059...,
  but it’s only for projects that are trying to conform to the REUSE
  specification.

Please see:

https://src.fedoraproject.org/rpms/python-chardet/blob/rawhide/f/python-char...
    https://src.fedoraproject.org/rpms/python-chardet/blob/rawhide/f/get_source

for a straightforward example of how you can obtain a “filtered” source
  archive.

- While this doesn’t affect the correctness of the License field,

# Apache-2:
    #  chainer/functions/pooling/roi_average_pooling_2d.py
    #  chainer/functions/pooling/roi_max_pooling_2d.py
    #  chainer/functions/pooling/roi_pooling_2d.py

should be

# Apache-2.0 AND MIT

since these files contain work under both licenses.

- Code in chainer/links/caffe/ is BSD-2-Clause (or is it just
  chainer/links/caffe/protubuf3?); either way, the License needs an “AND
  BSD-2-CLause” term.

- It would be best to add something like:

# Some code forked from an unknown version of Keras appears in:
    #   - chainer/initializer.py
    #   - chainer/initializers/uniform.py
    #   - chainer/initializers/orthogonal.py
    #   - chainer/initializers/normal.py
    Provides:       bundled(python3dist(keras))

to the python3-chainer subpackage to document that some code is “kind of
  bundled” from that project.

- This is a matter of opinion, so no change is required, but I think that
  %{pypi_name} and %{pypi_version} really clutter the spec file with
  unnecessary indirection. You can write the version directly as

Version:        7.8.1

and refer to it elsewhere as %{version}; and for %{pypi_name}, why not just
  write chainer? I don’t think macro-izing the name of the package really
  provides meaningful spec file reusability, certainly not enough to justify
  all the extra syntax.

Again, you’re not required to listen to this suggestion.

- There is no need to number the source and patch; you can just write

Source:         …
    Patch:          …

- Consider simplifying the Source URL from

%{forgeurl}/archive/refs/tags/v%{version}.tar.gz#/%{pypi_name}-%{version}.tar.gz

to

%{forgeurl}/archive/v%{version}/%{pypi_name}-%{version}.tar.gz

or

%{forgeurl}/archive/v%{version}/chainer-%{version}.tar.gz

(but, as noted earlier, I think you will need to use a script to download
  this URL and filter out the appveyor/ directory, so this URL won’t appear
  directly in the spec file except perhaps in a comment.)

- Since upstream doesn’t use tox, I don’t think it makes sense to write:

%pyproject_buildrequires -t

Normally you could write

%pyproject_buildrequires -x test

since the project has a “test” extra, but in this case it pins an old pytest
version and tries to bring in the deprecated https://pypi.org/project/mock/, so
that won’t do. Just write

%pyproject_buildrequires

and stick with the manual

BuildRequires:  pytest

you already have, which would be better written as

BuildRequires:  python3dist(pytest)

or

BuildRequires:  %{py3_dist pytest}

- This was probably always unnecessary, and is definitely unnecessary with
  pyproject-rpm-macros, which don’t even package egg-info (they use dist-info
  instead). Please remove it:

# Remove bundled egg-info
    rm -rf chainer.egg-info

- The %python_provide macro is obsolete; please remove this:

%{?python_provide:%python_provide python3-chainer}

https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_deprecate...

- You can avoid repeating the description text by writing

%global _description %{expand: 
    Chainer is a Python-based deep learning framework aiming at flexibility.
    It provides automatic differentiation APIs based on the define-by-run
    approach (a.k.a. dynamic computational graphs) as well as object-oriented
    high-level APIs to build and train neural networks.}

and then

%description %{_description}

and

%description -n python3-chainer %{_description}

- You can and should use %pyproject_save_files effectively. Change

%pyproject_save_files chainer

to

%pyproject_save_files chainer chainermn chainerx onnx_chainer

and write

%files -n python3-chainer -f %{pyproject_files}

and remove

%{python3_sitelib}/chainermn
    %{python3_sitelib}/chainerx
    %{python3_sitelib}/onnx_chainer
    %{python3_sitelib}/chainer
    %{python3_sitelib}/chainer-%{version}.dist-info

Now that you’re using %pyproject_save_files correctly, all of the installed
  packages get import-smoke-tested, which reveals the need to add some
BuildRequires. I found that this worked:

# We need one of the mpi4py flavors for testing or test-importing some
modules
    # within chainermn. It could be either openmpi or mpich; since the former
is
    # ExcludeArch: %%{ix86}, we might as well choose the latter.
    BuildRequires:  mpich-devel
    BuildRequires:  python3-mpi4py-mpich
    # Needed for testing or test-importing onnx_chainer:
    BuildRequires:  %{py3_dist onnx}

[…]

%{_mpich_load}
    %pyproject_check_import
    %{_mpich_unload}

- I think I agree with the choice to write

%license LICENSE chainerx_cc/LICENSE.txt

in order to properly mark a copy of chainerx_cc/LICENSE.txt as a license file
  (even though the main LICENSE file is already handled and marked in
  .dist-info; see the “-l” option to %pyproject_save_files, documented at
  https://src.fedoraproject.org/rpms/pyproject-rpm-macros).

You might want to add chainer/links/caffe/LICENSE_caffe_pb2 as well.

- It’s correct that you’re doing an import-only “smoke test” if you’re not
  running the tests, but is it really impossible to run any of the tests? And
  if so, could you at least explain why in a spec-file comment?

I tried to run the tests and got a DeprecationWarning about use of
  pkg_resources. I couldn’t figure out any way to successfully ignore the
  warning – and since upstream is in a “maintenance only” mode with no further
  releases planned, and they have pinned an ancient version of pytest, I doubt
  they are going to fix this, so maybe this is the answer. Still, maybe it’s
  worth filing a bug upstream.

Note that I tried running tests in a virtualenv using a checkout out from the
  upstream GitHub repo:

$ python3 -m venv _e
    $ . _e/bin/activate
    $ pip install -e '.[test]'
    $ python -m pytest
    […]
    ModuleNotFoundError: No module named 'imp'
    $ pip install zombie-imp
    $ python -m pytest
    […]
    ModuleNotFoundError: No module named 'numpy.distutils'
    $ patch -p1 <
~/Downloads/review/2259226-python-chainer/srpm-unpacked/0001-Remove-osx-numpy-check.patch
    $ python -m pytest
    […]
    ModuleNotFoundError: No module named 'chainerx._testing'
    $ python -m pytest tests/chainer_tests/ -v
    collected 152931 items / 8 errors
    $ python -m pytest tests/chainer_tests/
--ignore=tests/chainer_tests/links_tests/caffe_tests/test_caffe_function.py
--ignore=tests/chainer_tests/functions_tests/array_tests/test_get_item.py
--ignore=tests/chainer_tests/exporters_tests/test_caffe.py
--ignore=tests/chainer_tests/test_function_and_function_node.py -v
    collected 152931 items
    [canceled with ^C after a couple thousand tests, which all seemed to be
    SKIPPED or FAILED]

So maybe it’s worth putting a bit more effort into running the tests. Either
  way, the “bit-rot” with fully supporting Python 3.12 is already becoming
  apparent. Just make sure you’re fairly confident the package is working
  properly as packaged, and you understand that you’ll probably be on your own
  to fix future compatibility issues given the state of upstream support.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "MIT License", "*No copyright* Creative
     Commons CC0 1.0", "BSD 3-Clause License", "Apache License 2.0 and/or
     MIT License", "BSD 2-Clause License", "*No copyright* Creative Commons
     Attribution". 1712 files have unknown license. Detailed output of
     licensecheck in /home/ben/Downloads/review/2259226-python-
     chainer/licensecheck.txt

- Licensecheck detects “Creative Commons Attribution” in
       chainer/links/model/vision/vgg.py; this is spurious, and refers to the
       licenses of downloadable models that are not included in the package.
     - Some file in appveyor/ are CC0-1.0. This license is not-allowed for code
       in Fedora (even only in the source RPM!); see Issues.
     - A BSD-2-Clause license term for chainer/links/caffe/ is needed; see
       Issues.

[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.

This is no longer a hard requirement,
     https://gitlab.com/fedora/legal/fedora-legal-docs/-/issues/64, but it is a
     useful practice.

[x]: Package contains no bundled libraries without FPC exception.

It would be nice to treat code copied and forked from Keras as a case of
     bundling; see Issues.

[x]: Changelog in prescribed format.
[!]: Sources contain only permissible code or content.

Sources contain CC0-1.0-licensed AppVeyor code; see Issues.

[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines

(unless otherwise noted)

[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: The License field must be a valid SPDX expression.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 4306 bytes in 1 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[-]: Binary eggs must be removed in %prep
     Note: Cannot find any build in BUILD directory (--prebuilt option?)
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python

Some improvements are suggested in Issues.

[x]: Package contains BR: python2-devel or python3-devel
[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.

- Downstream-only patch is adequately justified within the patch file; it
       would be nice to reproduce this in a spec-file comment.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.

Rpmlint
-------
Checking: python3-chainer-7.8.1-1.fc40.noarch.rpm
          python-chainer-7.8.1-1.fc40.src.rpm
============================= rpmlint session starts
=============================
rpmlint: 2.5.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmpegev8yl8')]
checks: 32, packages: 2

python3-chainer.noarch: W: python-missing-require setuptools
python3-chainer.noarch: W: python-missing-require typing-extensions
python3-chainer.noarch: W: python-missing-require filelock
python3-chainer.noarch: W: python-missing-require numpy
python3-chainer.noarch: W: python-missing-require protobuf
python3-chainer.noarch: W: python-missing-require six
 2 packages and 0 specfiles checked; 0 errors, 6 warnings, 7 filtered, 0
badness; has taken 2.0 s

Rpmlint (installed packages)
----------------------------
============================ rpmlint session starts
============================
rpmlint: 2.5.0
configuration:
    /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
checks: 32, packages: 1

1 packages and 0 specfiles checked; 0 errors, 0 warnings, 9 filtered, 0
badness; has taken 0.9 s

Source checksums
----------------
https://github.com/chainer/chainer/archive/refs/tags/v7.8.1.tar.gz#/chainer-...
:
  CHECKSUM(SHA256) this package     :
62ca7b1933cf76d65ee9441b7f71d747cbbf41529148552efdaeeaf0a31822f2
  CHECKSUM(SHA256) upstream package :
62ca7b1933cf76d65ee9441b7f71d747cbbf41529148552efdaeeaf0a31822f2

Requires
--------
python3-chainer (rpmlib, GLIBC filtered):
    python(abi)
    python3.12dist(filelock)
    python3.12dist(numpy)
    python3.12dist(protobuf)
    python3.12dist(setuptools)
    python3.12dist(six)
    python3.12dist(typing-extensions)

Provides
--------
python3-chainer:
    python-chainer
    python3-chainer
    python3.12-chainer
    python3.12dist(chainer)
    python3dist(chainer)

Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24
Command line :/usr/bin/fedora-review -b 2259226 --mock-options=--dnf
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Shell-api, Generic
Disabled plugins: fonts, Java, C/C++, Haskell, Ocaml, SugarActivity, PHP, R,
Perl
Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #6 from [e-mail] ---
I lost this review when my email changed.
Looks like this will be impacted by the change to numpy 2

AttributeError: `np.sctypes` was removed in the NumPy 2.0 release. Access
dtypes explicitly instead.
error: Bad exit status from /var/tmp/rpm-tmp.iggG50 (%check)
    Bad exit status from /var/tmp/rpm-tmp.iggG50 (%check)

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

Ben Beasley  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |
           Assignee|[e-mail]     |[e-mail]
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |WONTFIX
        Last Closed|                            |2025-10-23 11:10:58

--- Comment #7 from Ben Beasley  ---
I just noticed that this review is still open and assigned to me. I had
entirely forgotten about it.

It looks like chainer entered a “maintenance phase” three years ago; even that
seems to have been an optimistic name, as there have been no commits whatsoever
since then. In particular, it looks like they never attempted numpy 2.0
support, so I think this won’t be a good candidate for packaging in Fedora at
this point.

In any case, since your employer-based Bugzilla email address has changed and
the original email is disabled since the original submission, I think you would
need to open a fresh issue if you want to pursue this. I’m going to take the
liberty of going ahead and closing this particular review request.

[Bug 2259226] Review Request: python-chainer - A flexible framework of neural networks

https://bugzilla.redhat.com/show_bug.cgi?id=2259226

--- Comment #8 from [e-mail] ---
Thanks for the feedback!
If/when I need this i will open up a new request.