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

Specify all dependencies of sagelib in its pyproject.toml #37446

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

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Feb 24, 2024

We specify all dependencies of sage-the-library it its pyproject.toml.
For external dependencies we use the format proposed by PEP 725. For additional dev, test, doc dependencies we use dependency groups as introduced in PEP 735.

While both PEP's are still in draft, there is already some support for them in external tooling, which we can use to e.g. automatically create the conda environment based on this information or create proper requirements lock files, see #37447. But you may take the perspective that at the moment it's just a convienent way to track this metadata information. In the future, sage-the-distribution can read this information of sage-the-library (if the people interested in its development find it worth).

Github now will also be able to pick up the python dependencies correctly, making #36999 obsolete.

We add this information to a new pyproject.toml file in the root to be independent of the disputed PRs:

📝 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 the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 24, 2024
@tobiasdiez tobiasdiez mentioned this pull request Feb 25, 2024
5 tasks
@dimpase
Copy link
Member

dimpase commented Feb 26, 2024

#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...

@tobiasdiez
Copy link
Contributor Author

#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).

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

dimpase commented Feb 27, 2024

@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...

@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 27, 2024
@roed314
Copy link
Contributor

roed314 commented Mar 14, 2024

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.

@dimpase
Copy link
Member

dimpase commented Mar 14, 2024

is this still a disputed PR?

@mkoeppe
Copy link
Member

mkoeppe commented Mar 14, 2024

This PR has obviously neither seen sufficient development, nor any meaningful review. It cannot be merged in this form.

It creates a pyproject.toml in the SAGE_ROOT, which I have objected to elsewhere.

  • it declares a build-backend = "setuptools.build_meta", which obviously does not work -- there is nothing that backs it, neither a setup.cfg, setup.py, nor [tools.setuptools] information.
  • it duplicates version information, violating our long-standing policy and practice of single-sourcing version information.

To summarize: -1, this is deliberate noise. please close.

@tobiasdiez
Copy link
Contributor Author

Setting this to positive review based on:
👍 me (as the author)
👍 @dimpase
👎 @mkoeppe

Copy link

github-actions bot commented Apr 11, 2024

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

@culler
Copy link
Contributor

culler commented Apr 12, 2024

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:

We add this information to a new pyproject.toml file in the root to be independent of the disputed PRs: ...

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.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 12, 2024

Thanks @culler for the clear analysis. Your vote brings us to:

Setting back to "needs review".

@dimpase
Copy link
Member

dimpase commented Apr 12, 2024

@orlitzky @tornaria - please have a look

@tobiasdiez
Copy link
Contributor Author

The primary objection to this PR is that is adds a pyproject.toml file in SAGE_ROOT.

So you would prefer if I were to modify the pyproject.toml file in the src directory instead?

@tobiasdiez
Copy link
Contributor Author

tobiasdiez commented Apr 18, 2024

@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.

@saraedum
Copy link
Member

@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.

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 :)

@tobiasdiez
Copy link
Contributor Author

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.

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.

That said, I think it's obvious that everybody would like to see a consensus on the technical discussion here :)

I agree. Maybe someone could start by explaining why creating a top-level file pyproject.toml is so controversial.
As a data point, the following projects have a pyproject file in their root only specifying metadata, ruff or other tools configs - and not being used for the installation of an actual python program. They are all reasonably popular (almost 300.000 stars combined), showing that such a metadata-pyproject file is not uncommon and normally doesn't lead to problems.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
… '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
@roed314
Copy link
Contributor

roed314 commented Apr 20, 2024

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.

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.

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.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 20, 2024

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)

@roed314
Copy link
Contributor

roed314 commented Apr 20, 2024

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.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 20, 2024

the bullet points below that sentence, no?

@roed314
Copy link
Contributor

roed314 commented Apr 20, 2024

the bullet points below that sentence, no?

Ok, I was confused. Those looked like specific objections about the details of what went into the pyproject.toml, rather than objections to having it at all. For the version information that @tobiasdiez is including in pyproject.toml that you don't want to see duplicated, where is it stored now?

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 25, 2024
… '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
@mkoeppe
Copy link
Member

mkoeppe commented Jul 27, 2024

This should be closed as a duplicate.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 31, 2024
…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):
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 2, 2024
…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):
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 3, 2024
…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):
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 s: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants