spec: https://fed500.fedorapeople.org/python-pymor.spec srpm: https://fed500.fedorapeople.org/python-pymor-2024.2.0-1.fc41.src.rpm description: pyMOR is a software library for building model order reduction applications with the Python programming language. All algorithms in pyMOR are formulated in terms of abstract interfaces, allowing generic implementations to work with different backends, from NumPy/SciPy to external partial differential equation solver packages. fas: fed500 Reproducible: Always
Copr build: https://copr.fedorainfracloud.org/coprs/build/9080030 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2368534-python-pymor/fedora-rawhide-x86_64/09080030-python-pymor/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.
Some quick preliminary comments: - the license indicates there are missing licenses from bundled third-party e.g. `src/pymor/algorithms/genericsolvers.py` (this one is BSD-3-Clause). On the spec side at least, the `License` field should include all licenses in the effective wheel (still haven't done the full license review to check the others) - upstream could use the new PEP639 format to clarify the license bundling as SPDX. This would also allow to include multiple license files in the metadata e.g. `LICENSE` and `LICENSE-scipy`. Could you try to convince upstream to migrate to this (requires bumping minimum hatchling version) - can you document why pytest is not run?
(In reply to Cristian Le from comment #2) > Some quick preliminary comments: > - the license indicates there are missing licenses from bundled third-party > e.g. `src/pymor/algorithms/genericsolvers.py` (this one is BSD-3-Clause). On > the spec side at least, the `License` field should include all licenses in > the effective wheel (still haven't done the full license review to check the > others) https://github.com/pymor/pymor/pull/2431 > - upstream could use the new PEP639 format to clarify the license bundling > as SPDX. This would also allow to include multiple license files in the > metadata e.g. `LICENSE` and `LICENSE-scipy`. Could you try to convince > upstream to migrate to this (requires bumping minimum hatchling version) https://github.com/pymor/pymor/pull/2429 > - can you document why pytest is not run? Missing pytest-notebook dependency. Other notes ann subpackage requires pytorch which does not install at present: https://bugzilla.redhat.com/show_bug.cgi?id=2368610 An updated build is at: https://koji.fedoraproject.org/koji/taskinfo?taskID=133224743 but waiting for upstream to respond to pull requests and packaging pytest-notebook.
(In reply to Benson Muite from comment #3) > > - can you document why pytest is not run? > Missing pytest-notebook dependency. How about skipping those test with something like `-k not` or probably deleting `docs/test_tutorials.py`? Running the tests in `src/pymortests` should still be fine right? > but waiting for upstream to respond to pull requests and packaging > pytest-notebook. Ah, that would be even better :). But I see some worrying issues and unmerged PRs in upstream there. There might be some alternatives like `nbval`, but I haven't checked the environment on these in a long time.
Some other things I've found: - The sdist does not have `conftest.py`. That might be needed for running the tests. - `docs/test_tutorials.py` is not actually run by default, so you should be generally safe - All files seems to have `#!/usr/bin/env python3`, you might want to run `%py3_shebang_fix` on all of them, but frankly upstream should not have those shebangs, and instead document using `python3 -m`. The shebang approach does not guarantee that they are running on the same package version - Can/should `pymordemos` be split to a different sub-pkg? We could do it in downstream-only
Thanks. Will implement these changes. Demos are meant to be installed together with the main library: https://docs.pymor.org/latest/getting_started.html#running-demo-scripts Started process for test dependency: https://bugzilla.redhat.com/show_bug.cgi?id=2368625 Guess https://www.librom.net/ will have fewer packaging issues.
Dependencies seem to ahve been resolved, shall we get back to this review?