Bug 2368534

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?