-
-
Notifications
You must be signed in to change notification settings - Fork 478
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 dependency groups to pyproject.toml
#37446
base: develop
Are you sure you want to change the base?
Conversation
cf0b6d7
to
2e2c641
Compare
#37482 - is a peculiar PR, just cherry-picking commits from here, a PR on which I can't even comment, and positively reviewed by the PR author himself... |
It's not even a cherry-picked commit. Matthias copied a subset of the code changes of this PR, put's it into a different place that is more to his liking and slaps a "positive review" label on it. He did the same with #37452, and insisted that it's "positively review" even after I've removed that label (and explained in #36503 (comment) why the new location is not working for me). @roed314 @williamstein sorry to bother you with this, but my understanding is that this is a clear misuse of the "positive review" label and, since done multiple times on purpose and not by accident, an abuse of his admin/triage rights. Speaking of abuse of admin rights, Matthias continues to flag my review comments as "resolved" - sometimes without even a response to the question (#37351 (review)). Normal users without admin rights cannot mark them as "unresolved" again, effectively hiding them from other reviewers. This problematic conduct has been mentioned to Matthias at multiple instances before (and if my memory serves me well even by members of the sage abuse team). |
@mkoeppe has blocked me on GH - while this is sorted out, we can proceed. And if it's not sorted out, we can block him, why not... |
The Sage Code of Conduct Committee is setting this back to needs review for now based on it being part of @dimpase's reaction to our decision on #36181, #36561, #36676, #36951, #36964, #37351, #36999, and #36941. It can, of course, be granted positive review at a future date, and will be subject to the policy on disputed PRs as normal. |
is this still a disputed PR? |
This PR has obviously neither seen sufficient development, nor any meaningful review. It cannot be merged in this form. It creates a
To summarize: -1, this is deliberate noise. please close. |
The part that declares external dependencies according to draft PEP 725 is valuable and interesting. I am hoping to use it with the skeleton generator for the modularized Sage packages in pyodide, #34539. I have split it out as
|
we don't really practice "single source version information", it can't therefore "violate" it. |
Documentation preview for this PR (built with commit c7beb90; changes) is ready! 🎉 |
The primary objection to this PR is that is adds a pyproject.toml file in SAGE_ROOT. The explanation for why this was done is:
which seems to not have to do with the design, but rather with the PR review process. There is some consensus that the ideas in this PR are useful. There is some evidence that they have not been fully worked out yet. And attempts to incorporate some of the ideas into a working PR without adding a pyproject.toml file in SAGE_ROOT have been summarily rejected and attacked on what seem to be personal grounds. I think this is an unfinished proposal which might prove to be useful if people could manage to cooperate. But there seems to be little hope for that happening. Because of this state of affairs, I vote -1. I don't see how merging this PR in the current context could possibly improve Sage. |
Thanks @culler for the clear analysis. Your vote brings us to:
Setting back to "needs review". |
So you would prefer if I were to modify the pyproject.toml file in the src directory instead? |
@kwankyu As the copyright holder, I'm objecting that my work is used in the form of #37482 and have hence removed the positive review label there. Instead, I propose to close #37482 until the discussion in this PR here came to an end. If there are things that should be improved, I would like to do this in this PR. For example, I'm still waiting for a response to my earlier question #37446 (comment) to one of the reviewers here. Depending on the outcome, this may or may not make #37482 obsolete without creating additional friction for me. |
I'm not a lawyer but I don't think that's how the GPL works. When you published your code here, you made it available under the GPL. You still hold the copyright but that does not give you the right to stop others to use your code within the terms of the GPL. That said, I think it's obvious that everybody would like to see a consensus on the technical discussion here :) |
Sure, but the GPL requires that modifications and the original copyright holder are clearly marked in a prominent place. Perhaps, you would even need to state that sagemath is now a derived work of my fork - which is just absurd. But please not start a legal war on top of the already complicated enough situation.
I agree. Maybe someone could start by explaining why creating a top-level file pyproject.toml is so controversial. |
… 'external' section according to draft PEP 725 <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> As done in sagemath#37482 (unbundled from sagemath#37446) for **sagemath-standard**. - This information will be used in the skeleton generator in pyodide/pyodide#4438 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37486 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee
Here's what I can see about the current status of giving credit:
I can't speak to the pyproject.toml question right now, though I hope others will be willing to. |
I have explained it in #37446 (comment) |
The comment you cite just says "It creates a pyproject.toml in the SAGE_ROOT, which I have objected to elsewhere," but does not include a pointer to where you gave reasons elsewhere. |
the bullet points below that sentence, no? |
Ok, I was confused. Those looked like specific objections about the details of what went into the |
… 'external' section according to draft PEP 725 <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> As done in sagemath#37482 (unbundled from sagemath#37446) for **sagemath-standard**. - This information will be used in the skeleton generator in pyodide/pyodide#4438 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37486 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee
This should be closed as a duplicate. |
…t PEP 725 (unbundled from sagemath#37446) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This is aspirational decoration for future use by skeleton generators by distributions such as conda, sage-the-distribution, pyodide. Split out from the disputed sagemath#37446, where it is bundled with a number of other changes, including: creating a `pyproject.toml` file in `SAGE_ROOT`, hardcoding versions of Python packages instead of using the existing `sage_bootstrap` infrastructure, etc. @roed314 @vbraun **The scope of PRs should be chosen to minimize friction, not to maximize friction.** sagemath#36726 (comment) Author: @mkoeppe, based on @tobiasdiez's sagemath#37446. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [X] The title is concise and informative. - [ ] 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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37482 Reported by: Matthias Köppe Reviewer(s):
…t PEP 725 (unbundled from sagemath#37446) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This is aspirational decoration for future use by skeleton generators by distributions such as conda, sage-the-distribution, pyodide. Split out from the disputed sagemath#37446, where it is bundled with a number of other changes, including: creating a `pyproject.toml` file in `SAGE_ROOT`, hardcoding versions of Python packages instead of using the existing `sage_bootstrap` infrastructure, etc. @roed314 @vbraun **The scope of PRs should be chosen to minimize friction, not to maximize friction.** sagemath#36726 (comment) Author: @mkoeppe, based on @tobiasdiez's sagemath#37446. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [X] The title is concise and informative. - [ ] 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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37482 Reported by: Matthias Köppe Reviewer(s):
…t PEP 725 (unbundled from sagemath#37446) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This is aspirational decoration for future use by skeleton generators by distributions such as conda, sage-the-distribution, pyodide. Split out from the disputed sagemath#37446, where it is bundled with a number of other changes, including: creating a `pyproject.toml` file in `SAGE_ROOT`, hardcoding versions of Python packages instead of using the existing `sage_bootstrap` infrastructure, etc. @roed314 @vbraun **The scope of PRs should be chosen to minimize friction, not to maximize friction.** sagemath#36726 (comment) Author: @mkoeppe, based on @tobiasdiez's sagemath#37446. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [X] The title is concise and informative. - [ ] 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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37482 Reported by: Matthias Köppe Reviewer(s):
pyproject.toml
The scope of this PR is now largely reduced, only adding a few dependency groups at the end of the pyproject.toml file. I've thus removed the "disputed" flag. (I can also close this PR and open a new one if this is preferred) |
I'll be slow to respond in the coming 5-6 days or so, as we're moving across the pond tomorrow. |
Following PEP 735, add a few groups of dependencies that one usually needs while developing sage (e.g. for tests or docs).
This info will then be used to generate the conda lock files and once support for PEP 735 is more widespread other tools will be able to use the metadata as well. Eg, once pypa/pip#12963 is implemented one can install all test dependencies with
pip install --groups test
.📝 Checklist
⌛ Dependencies