Add requirements-reference.txt#871
Conversation
Add loose version constraints to remaining tutorial requirements.txt files, generate requirements-reference.txt from PyPI, and check it in CI.
Remove extra trailing blank line in the Python dependencies section.
MakisH
left a comment
There was a problem hiding this comment.
Thank you for the contribution; this will be useful, and it comes at the right time.
My main suggestion, which will change a bit the code, is to split the requirements-reference.txt into one file per requirements.txt.
Other than that, most comments are regarding the version ranges. We do not need to document these in detail, but the comments here might be helpful in the future.
| # Not a pip requirements file (lists stdlib os, unpinned vtk); see develop naming quirk. | ||
| "flow-over-heated-plate/plot-final-interface-temperature-requirements.txt", |
There was a problem hiding this comment.
This should actually be a requirements.txt file. I wanted to tidy these plotting python scripts a bit.
Useful to have an exclusion list, I guess (mainly for unmaintained tutorials), but why does this need to be excluded?
There was a problem hiding this comment.
Done, moved to plot-final-interface-temperature/requirements.txt (dropped invalid OS), and removed the special exclusion.
There was a problem hiding this comment.
Good point, but the script itself should also be in that directory then.
You can also make a directory utils/: https://precice.org/community-contribute-to-precice.html#structure-of-a-tutorial
…ependency-versions-clean
Move the generator to tools/releasing/, emit sibling reference files instead of a root manifest, and clean up requirements from review feedback.
There was a problem hiding this comment.
The version ranges look fine. The system tests pass: https://github.com/precice/tutorials/actions/runs/29193941172?pr=871
I had a closer look at the script, here are some more comments.
| from packaging.requirements import Requirement | ||
| from packaging.specifiers import SpecifierSet | ||
| from packaging.utils import canonicalize_name | ||
| from packaging.version import Version |
There was a problem hiding this comment.
It looks like we also need a requirements.txt for this tool.
| Run from the repository root: | ||
| python3 tools/releasing/report_tutorial_requirements.py | ||
| python3 tools/releasing/report_tutorial_requirements.py --check |
There was a problem hiding this comment.
Some suggestions for the interface:
- I would name it
update-requirements-reference.py: dashes for consistency with most of our tools (yes, we can improve that), clearly point to these files. - It would be nice to accept a path to only update files in that directory. In the PR template, you added a step to do that, but one can only update all files at once.
- By default, all files will be updated, because the date/time is always different. This makes it difficult to see which of them actually changed in terms of versions. An option
--allwould be mostly intuitive to me (any better ideas?).
| if outdated: | ||
| print("ERROR: outdated requirements-reference.txt files:", file=sys.stderr) | ||
| for path in outdated: | ||
| print(f" - {path}", file=sys.stderr) | ||
| print( | ||
| "Run: python3 tools/releasing/report_tutorial_requirements.py", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
|
|
There was a problem hiding this comment.
I think that the outdated ones should only give a warning, not an error. We should only update them when making a new release.
| push: | ||
| branches: | ||
| - master | ||
| - develop | ||
| paths: | ||
| - '**/requirements.txt' | ||
| - '**/requirements-reference.txt' | ||
| - tools/releasing/report_tutorial_requirements.py | ||
| pull_request: | ||
| branches: | ||
| - master | ||
| - develop | ||
| paths: | ||
| - '**/requirements.txt' | ||
| - '**/requirements-reference.txt' | ||
| - tools/releasing/report_tutorial_requirements.py |
There was a problem hiding this comment.
I think that we should check:
- In a PR to
develop: That there arerequirements-reference.txtfiles: The first time a tutorial is added, it would be good to archive that information. - In a PR to
master(i.e., a release): That therequirements-reference.txtfiles are up to date: This is the time I would expect them to be updated.
I would say that the push event is not needed, then.
| # Not a pip requirements file (lists stdlib os, unpinned vtk); see develop naming quirk. | ||
| "flow-over-heated-plate/plot-final-interface-temperature-requirements.txt", |
There was a problem hiding this comment.
Good point, but the script itself should also be in that directory then.
You can also make a directory utils/: https://precice.org/community-contribute-to-precice.html#structure-of-a-tutorial
| @@ -0,0 +1,228 @@ | |||
| #!/usr/bin/env python3 | |||
| """ | |||
| Report resolved Python dependency versions for tutorial requirements.txt files. | |||
There was a problem hiding this comment.
Not sure if you only mean the files corresponding to tutorials cases, but I think we should cover every requirements.txt file in this repository. There is a list with exceptions, which is nice, but I think it should be empty by default, unless there is a good reason not to.
|
|
||
| def parse_requirement_line(line: str) -> str | None: | ||
| stripped = line.strip() | ||
| if not stripped or stripped.startswith("#") or stripped.startswith("--"): |
Summary
This PR implements #610 using the approach discussed after #749 and it is the rework of that:
requirements.txt fileskeep loose version ranges(for example numpy >1, <2, pyprecice~=3.0). This PR fills gaps where develop still had unpinned packages (~~39 files; ~ ~13 already had ranges).requirements-reference.txtrecords the exact PyPI versions that satisfy those ranges at generation time(Gemfile.lock style).tools/report_tutorial_requirements.pygenerates and checks that file from PyPI.check-requirements-reference.ymlruns--checkwhen requirements or the script change.Tutorial
run.sh scriptsare unchanged and still install from localrequirements.txt. The reference file is for reproducibility and releases, not for day-to-day installs.This is not tied to
reference_versions.yamland does not changetools/tests/requirements.txt(CI git refs are already handled by #857/#858).Test Plan
python3 tools/report_tutorial_requirements.py
python3 tools/report_tutorial_requirements.py --check
pre-commit run actionlint --files .github/workflows/check-requirements-reference.yml
pip install --dry-run -r elastic-tube-1d/fluid-python/requirements.txt
Closes #610