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

Rename install-requires.txt to version_requirements.txt #36999

Merged
merged 6 commits into from
Apr 8, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jan 3, 2024

As discussed in #36982:

#36982 (comment)

Notes for reviewers:

📝 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

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

There it is "dependencies"?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

That's right. The term "install-requires" has been eliminated.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

Then you made up the name "version_requirements.txt", instead of using "dependencies.txt"?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

That's right.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

(we may come up with a name better than both, but I have no idea now...)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

it needs to end with "_requirements.txt" so that GitHub finds it (see link in PR description). I didn't have a better idea what to stick in front.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

Simply "install_requirements.txt"?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

I cannot come up with an obviously better name than "version_requirements.txt". This file is about version requirements on dependencies (pip packages on pypi). So other possible names are "dependency_requirements.txt" or "pip_requirements.txt". No strong opinion anyway...

@dimpase
Copy link
Member

dimpase commented Feb 22, 2024

I still don't get why we cannot just use requirements.txt.

Same format, essentially the same purpose, ease of conversion to pip package.

If there is any need to prepend foo_ to this name it can be done on the fly by tools.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

Perhaps because an spkg is not a python package?

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

Another suggestion: "package_requirements".

@dimpase
Copy link
Member

dimpase commented Feb 22, 2024

Perhaps because an spkg is not a python package?

These are Python packages we are talking about, these which are vendored by Sage, i.e. made into spkg packaged as tarfiles or wheels. Package names in install-requires.txt are not spkg names, these are Python package names.
You can rename install-requires.txt to requirements.txt, remove package_version.txt and checksums.ini, and usually
such a package can still be installed, as a PyPI package, by-passing Sage's tarballs/wheels.

If it's a standard package, it's an illegal operation under the current policy; as you know I called a sage-devel vote on changing this policy...

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

I agree that "requirements.txt" seems the best choice. According to the developer guide, presence of "requirements.txt" is used to differentiate "pip" spkg from "normal" and "wheel" spkg. How about using presence of "checksums.ini" for this purpose? Do I miss something? Is there other reason that "requirements.txt" cannot be a choice?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

I still don't get why we cannot just use requirements.txt.

@dimpase I explained it to you in #37401 (comment); complete the review there, please

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

According to the developer guide, presence of "requirements.txt" is used to differentiate "pip" spkg from "normal" and "wheel" spkg.

Yes, that's right.

How about using presence of "checksums.ini" for this purpose?

This could be done (best after the refactoring in #36740, which needs review), but I would still not use requirements.txt for the acceptable version ranges of normal/wheel packages.

Edit: The part of #36740 needed for that is now split out as #37430 (needs review)

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 22, 2024

Because requirements.txt means something else. It is the name one uses for version pinnings, as in pip install -r requirements.txt.

But you can use version ranges too in the file.

install-requires is the traditional name (in setuptools) for declaring dependencies.

Yes. But as far as I understand, install-requires in an spkg specifies the "dependency" of the spkg, which is the pip package that the spkg installs. The "dependency" can be well described by the "requirements.txt" file. (the spkg "requires" the specified versions.)

Anyway, "version-requirements.txt" is slightly different from "requirements.txt". I feel that "version" prefix emphasizes the version too much: "the spkg requires the version(?)"...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

Because requirements.txt means something else. It is the name one uses for version pinnings, as in pip install -r requirements.txt.

But you can use version ranges too in the file.

I should have written: requirements.txt ... is the file name one uses for version pinnings, as in pip install -r requirements.txt.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

In Python packaging, one distinguishes "abstract dependencies" (install-requires) vs "concrete dependencies" (requirements.txt). See e.g. https://martin-thoma.com/python-requirements/#abstract-dependencies

That's why using the name requirements.txt for declaring our acceptable version ranges would be bad -- it would confuse anyone who is familiar with Python packaging practices.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2024

install-requires is the traditional name (in setuptools) for declaring dependencies.

Yes. But as far as I understand, install-requires in an spkg specifies the "dependency" of the spkg, which is the pip package that the spkg installs.

No, that's not a good way of viewing the relation of the spkg and the pip package. The spkg does not "depend" on it. It "represents" it.

I chose the name "install-requires.txt" to mean: If a Python distribution needs to declare this package as an "install-requires", use the contents of this file for it. And that's what is done when we create pkgs/*/setup.cfg from the corresponding setup.cfg.m4.

By the way, we are also generating "pkgs/*/requirements.txt" files such as pkgs/sagemath-standard/requirements.txt. The versions in there are pinned (using the information from package-version.txt files).

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 23, 2024

In Python packaging, one distinguishes "abstract dependencies" (install-requires) vs "concrete dependencies" (requirements.txt). See e.g. https://martin-thoma.com/python-requirements/#abstract-dependencies

That's why using the name requirements.txt for declaring our acceptable version ranges would be bad -- it would confuse anyone who is familiar with Python packaging practices.

Thanks for the read. I see now why you reject the name requirements.txt: In python packagaing, install-requires and requirements.txt have (loosely) separate uses.

I chose the name "install-requires.txt" to mean: If a Python distribution needs to declare this package as an "install-requires", use the contents of this file for it. And that's what is done when we create pkgs/*/setup.cfg from the corresponding setup.cfg.m4.

This explains why I got confused. The name "install-requires.txt" cannot be explained within spkg context. You see the spkg as a "dependency" of the Python distribution, and the content of install-requires.txt is supposed to be used by the Python distribution. I was thinking within spkg context.

Now the Python distribution uses pyproject.toml, and the file uses the name "dependencies" instead of old "install-requires". Then logically, you want to replace "install-requires.txt" with "dependencies.txt", but "dependencies" is already being used for spkg dependencies. Hence we need a new name. Am I right?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 23, 2024

logically, you wan to replace "install-requires.txt" with "dependencies.txt", but "dependencies" is already being used for spkg dependencies. Hence we need a new name. Am I right?

Exactly that. "install-requires" was an idiosyncratic name only used for one purpose in Python packaging, and hence it was unambiguous. "dependencies" is not just already used, it's also too generic.

@jhpalmieri
Copy link
Member

I'm posting to record a vote of -1 on behalf of Tobias Diez.

@mkoeppe mkoeppe requested a review from kiwifb March 14, 2024 20:33
@kiwifb
Copy link
Member

kiwifb commented Mar 14, 2024

It seems to me the dispute is really about the naming. In any case we need to move from the current name +1 for me.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 14, 2024

Still +1 from me. version_requirement.txt is to replace install-requires.txt, which is different from requirement.txt.

@NathanDunfield NathanDunfield self-requested a review March 15, 2024 15:57
Copy link
Contributor

@NathanDunfield NathanDunfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I agree @kwankyu in #36999 that version_requirements.txt is the best choice of name here.

@roed314
Copy link
Contributor

roed314 commented Mar 15, 2024

Based on @kwankyu's suggestion and discussion by the new Code of Conduct Committee, I'm setting this ticket to Needs Review so that all the disputed tickets start with a clean slate prior to being update based on voting. Starting at midnight US/Pacific time tonight anyone involved should feel free to set the status to positive review or needs review in accordance with the new policy.

@NathanDunfield
Copy link
Contributor

Per the comment by @roed314, I have set to positive review counting:

+1: @mkoeppe, @kwankyu, @kiwifb, and myself
-1: @tobiasdiez, @dimpase

@vbraun
Copy link
Member

vbraun commented Mar 20, 2024

Please try building from a clean checkout, I'm getting lots of sage-spkg-info: unknown package xz for all packages

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 1, 2024
…nts.txt`

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

As discussed in sagemath#36982:
- the name "install-requires"  has become outdated with the transition
to the new names set by the `pyproject.toml` format (see https://setupto
ols.pypa.io/en/latest/userguide/dependency_management.html#declaring-
required-dependency, compare the tabs "pyproject.toml" vs. "setup.cfg")
- this will help with sagemath#35890, as
GitHub will be able to just read the `version_requirements.txt` files

sagemath#36982 (comment)

Notes for reviewers:
- **This is a simple, limited-scope improvement and not intended as a
long-term commitment to the format of the files in build/pkgs/....**
- PRs sagemath#37430, sagemath#37350, sagemath#36740 remove direct access to build/pkgs in favor
of using the sage_bootstrap API (sage --package).

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#37401

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36999
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Nathan Dunfield
Copy link

github-actions bot commented Apr 1, 2024

Documentation preview for this PR (built with commit 918448a; changes) is ready! 🎉

@kwankyu kwankyu mentioned this pull request Apr 4, 2024
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 8, 2024
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
[python-flint](https://github.com/flintlib/python-flint) is an optional
dependency of the upcoming SymPy 1.13
(https://groups.google.com/g/sympy/c/S3LL7hYvD_A) @oscarbenjamin

See also
- sagemath#36641

<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#37222
- Depends on sagemath#36999 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37224
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit 2f5d9f1 into sagemath:develop Apr 8, 2024
13 of 33 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: build disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants