Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add requirements files for testing, linting, and documentation #37461

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Feb 25, 2024

We add *-requirements.txt files for locking the dependencies used in ci runs. The need for such a locking was raised on the mailing list. Here we following the (or, rather, a) standard approach to this problem as outlined by @mkoeppke in https://groups.google.com/g/sage-devel/c/MIU-xo9b7pc/m/cD7__bOUAAAJ:

In the Python (pip) world:
- Version constraints of dependencies of a Python project are declared in pyproject.toml [project] dependencies (https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#declaring-required-dependency); or, equivalently. in the older formats: setup.cfg install-requires, or setup.py install_requires
- Pinning to specific versions is done using a file "requirements.txt" (https://pip.pypa.io/en/stable/reference/requirements-file-format/)
- This file can be created and updated by using "pip freeze".

The metadata is store in the pyproject.toml file added in #37446, the locked down requirements files are stored in the folder requirements (which seems to be the usual convention). The only difference is that we use a simple wrapper around pip compile ( https://github.com/jazzband/pip-tools). This is more convienent than pip freeze as the dependencies do not have to be installed.
Since the PR #37446 is sadly also "disputed", we only commit the resulting requirements files, the wrapper to update them will be another PR.
The resulting setup is completely in line with the standards set by other "huge" python projects, eg https://github.com/scipy/scipy/tree/main/requirements or https://github.com/scikit-learn/scikit-learn/tree/main/build_tools/azure.

The generated requirement files are then used in the ci runs. As a positive side-effect, github will parse these requirement files and show them correctly in the dependency graph (this closes #35890).

Marking as blocker because I think it's important enough to get into the beta.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe added disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ and removed p: blocker / 1 labels Feb 25, 2024
Copy link

github-actions bot commented Mar 10, 2024

Documentation preview for this PR (built with commit b153f9d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 14, 2024

@roed314 @jhpalmieri I set "disputed" here because when the PR was opened, I was not able to comment because Tobias had blocked me without explanation or giving notice to the sage-abuse committee. It appears that now I can comment, so I am removing "disputed".
There has not been any review yet here, so conducting a vote would seem premature.

@mkoeppe mkoeppe removed the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Mar 14, 2024
@mkoeppe
Copy link
Member

mkoeppe commented Mar 14, 2024

I have not looked at this proposal in more detail. But I have to note that project will not need "docs-requirements" because we have a proper solution for building the docs in:

@mkoeppe
Copy link
Member

mkoeppe commented Mar 17, 2024

folder requirements (which seems to be the usual convention)

Some more examples please - the PR description only gives scipy as an example that uses this folder (it was only added a few months ago).

Overall, such changes of the top-level structure (what's in SAGE_ROOT) would probably benefit from a broader discussion.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 17, 2024

As a positive side-effect, github will parse these requirement files and show them correctly in the dependency graph (this closes #35890).

It wouldn't close PR #35890 - as it only takes care of a few packages, whereas #35890 is about the full dependency graph including non-Python packages. Best to remove this closing reference from the PR description.

@mkoeppe mkoeppe removed this from the sage-10.3 milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete our GitHub dependency graph (javascript help needed)
3 participants