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 # sage_setup: distribution directives to all files, remove remaining # coding: utf-8 #36964

Merged
merged 88 commits into from
Apr 12, 2024

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Dec 25, 2023

These directives at the top of the file inform developers about the intended assignment of modules to pip-installable distributions.

As of this PR, there should be no change to the existing distributions (sagemath-categories...) nor the monolithic build of the Sage library.

📝 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

(cd src && for d in $(find sage -name __pycache__ -prune -o -type d -print); do sed -i.bak "/from *[.].*import/s/from /from $(echo $d | sed 's,/,.,g')/;s/[.] import / import /;" $d/all*.py; done)
…o if ! grep -q 'del lazy_import' $a; then echo 'del lazy_import' >> $a; fi; done
SageMath version 10.2.rc2, Release Date: 2023-11-12
…o if ! grep -q 'del install_doc' $a; then echo 'del install_doc' >> $a; fi; done
@roed314
Copy link
Contributor

roed314 commented Apr 11, 2024

@tobiasdiez According to the policy on disputed tickets, when you change the review status on a disputed ticket you must specify the votes of enough developers to support the change, and you should not remove the disputed label. Do not do this again.

@roed314 roed314 added s: positive review disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ and removed s: needs review labels Apr 11, 2024
@jhpalmieri
Copy link
Member

+1 from me

@saraedum
Copy link
Member

@vbraun please do not merge this one yet. It depends on two disputed PRs that have not received positive review yet.

@vbraun vbraun merged commit 6ecb1d8 into sagemath:develop Apr 12, 2024
17 of 37 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Apr 12, 2024
@mkoeppe mkoeppe deleted the distribution_directives branch April 12, 2024 23:25
@vbraun
Copy link
Member

vbraun commented Apr 13, 2024

Sorry, but thats too late now. Please don't set branches to "positive review" if they are not ready to be merged, that is not how git works. You can open a new PR that reverts this one.

@saraedum
Copy link
Member

Sorry, but thats too late now. Please don't set branches to "positive review" if they are not ready to be merged, that is not how git works. You can open a new PR that reverts this one.

Sorry for the misunderstanding on how you are handling disputed PRs here. I'll try to propose some amendments to the disputed PR procedure to make sure we get this right in the future :)

@@ -1,3 +1,4 @@
# sage_setup: distribution = sagemath-graphs
Copy link
Collaborator

Choose a reason for hiding this comment

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

this does certainly not belong into graphs. Graphs do not come with a root, and trees in the sense of graph theory do not come with an embedding (which may or may not be the case with trees in combinatorics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you read the description of the scope of the sagemath-graphs distribution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where can I read it? I asked for it before, but you said that the whitepaper is a submission to ICMS, which I also find very hard to understand.

Why are posets in graphs? Why are symmetric functions in combinat? This makes no sense to me at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just saw https://groups.google.com/g/sage-devel/c/kiB32zP3xD4, which was a thread in which only you and David Roe participated. I did not see it before. There, you write


Packages that are named after a basic mathematical structure but may cover a wide range of generalizations/applications of this structure. People who work in a specialized research area will of course recognize what structures they need. But the down-to-earth naming also creates discoverability for a broader audience. Not many more distribution packages than these 7 are needed:


I cannot make sense of this. Please explain, or point to the discussion where this has been explained.

Where are the dependencies explained. For example, bijectionist.py has the annotation

# sage_setup: distribution = sagemath-combinat
# sage.doctest: needs sage.numerical.mip

but, of course, without the solver it is completely unusable.

On the other hand findstat.py and sloane.py are in no distribution at all. Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where can I read it? I asked for it before, but you said that the whitepaper is a submission to ICMS, which I also find very hard to understand.

@mantepse What is hard to understand?

You are referring in #36676 (comment), where I offered to send you a copy of the paper by email if you are interested, and included the link to #29705, which has the links to the previous writing to sage-devel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

... But I do find it extremely hard to understand that a design decision in sage that is not yet agreed upon would be topic in a plenary session of a conference.

I read the paper. It relates the modularization project most of which is already made into sage.

How the sage library would end up split into distribution packages is in flux and subject to change. This PR is not the end of the story. For me, binary trees being in sagemath-graphs is perfectly valid. But If many would want to move binary trees to, say, sagemath-trees, that is also possible in principle. Design decisions along this line would be subject of future PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mantepse It's a bit hard for me to see what your points in the above comment may be. Consider rewriting it a bit so I don't have to guess?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit puzzled, I would say that it's the authors job to explain why a patch is an improvement. Also, I asked more precise questions on #36676, but you only answered

Don't fear. This has been thought through carefully, and you can read about it in the Sage developer's guide."

(Even though also @kcrisman said that the questions were legitimate and he couldn't answer them.)

But anyway, I'll repeat and slightly expand. Please excuse the length of the comment.

1.) Is there an example for someone who did not want to use sage because of some dependency of the math library? Or at least a possible reason? @kwankyu's comment above suggests that having something in the "wrong" distribution wouldn't be a big deal. But this begs the question: who profits from cutting the math library into pieces (which look very arbitrary to me and have a curious emphasis on discrete math topics)?

My fear would be that at some point there is a request not to use symbolics in some module, because Lisp is hard to install on some system.

(I don't think this fear is unjustified: in the section of the developer guide you pointed to, I find

The imports of the symbolic functions ceil() and floor() can likely be replaced by the artithmetic functions integer_floor() and integer_ceil().

OK, so some user of that module happily replaces the two functions. Now, I come along and would like to replace some other implementation by a call to something defined in symbolics. But that would be breaking a promise to the user, so it would be really hard to justify.

In fact, this happened to me already, in some sense. I noticed a function definition in sage.modular.multiple_zeta with misleading documentation, which could be replaced by a call to code in sage.combinat. However, this is already hard to do, because it might affect performance (which is a very valid point in my opinion). I think it would be extremely bad to make it even harder.

2.) If this is about dependencies on other software, why aren't the distributions named after these dependencies? (Of course, at some point dependencies might change, for example, there might be a switch from glpk to scip.)

Before I read - by chance - distribution = sagemath-graphs somewhere, I thought one would "modularize" things like the repl, user interfaces, and perhaps some low level stuff. But it seems to me now that this is really about the modularization of the mathematics.

Also, I find it hard to believe that it is about dependencies, because the stuff in abstract_tree.py and friends has no dependencies on external software (unless you want to LaTeX them, perhaps).

Copy link
Member Author

Choose a reason for hiding this comment

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

@mantepse I asked to consider rewriting this post: #36964 (comment)

Sorry, I cannot remember the circumstances. I find it hard to believe that I said I wasn't interested, but it is quite possible that at the time its relevance was not clear to me. Sagemath is not my only occupation.

I have no idea what conference ICMS refers to. But I do find it extremely hard to understand that a design decision in sage that is not yet agreed upon would be topic in a plenary session of a conference.

The ticket description in #29705 is a link list. The link which looks most relevant points to a discussion between you and David, which does not look like it has been voted on. I don't have the energy to read all of this. The new issue template asks for "concise" descriptions, I think a link list to discussions does not satisfy this requirement.

Just in case I missed something, I also clicked on "the goals" now. It is a discussion with some 100 messages. I cannot read that, and I find it hard to believe that this cannot be summarized. The remainder of the ticket description does not contain the string sagemath-graphs anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I cannot remember the circumstances. I find it hard to believe that I said I wasn't interested

@mantepse that's

where you wrote

I must admit that I don't really care much about a theory of dependencies.

in response to me pointing you to the relevant sections in the developer's guide. #36753 (comment)

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 13, 2024

... I'll try to propose some amendments to the disputed PR procedure to make sure we get this right in the future :)

Right. I once proposed: sagemath/sage-release-management#8. But perhaps it is not welcome for the release manager to change his workflow.

So perhaps we should separate the voting counting and the label changing. I propose to change the label at least a week later after the last counting. The author may use the interim to update his branch.

@saraedum
Copy link
Member

saraedum commented Apr 14, 2024

Right. I once proposed: sagemath/sage-release-management#8. But perhaps it is not welcome for the release manager to change his workflow.

I think that we hope for disputed PRs to be the rare exception going forward. So maybe there is no need to automate these things.

So perhaps we should separate the voting counting and the label changing. I propose to change the label at least a week later after the last counting. The author may use the interim to update his branch.

Yes, something like this would sound perfectly good to me. Maybe we should continue this discussion on sage-devel so we are not distracting from what this PR is actually about.

tobiasdiez pushed a commit to tobiasdiez/sage that referenced this pull request Apr 14, 2024
… to all files, remove remaining `# coding: utf-8`"

This reverts commit 6ecb1d8, reversing
changes made to 8ea5214.
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 17, 2024
    
sagemath#36964 was inappropriately merged, since two dependencies were still
disputed.  This PR attempts to revert the merge.  It was created by

```
git revert -m 1 6ecb1d8
```

### 📝 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 and checked the documentation
preview.

### ⌛ Dependencies

None, though it will also revert sagemath#36676 and sagemath#36951.
    
URL: sagemath#37796
Reported by: roed314
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 25, 2024
    
sagemath#36964 was inappropriately merged, since two dependencies were still
disputed.  This PR attempts to revert the merge.  It was created by

```
git revert -m 1 6ecb1d8
```

### 📝 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 and checked the documentation
preview.

### ⌛ Dependencies

None, though it will also revert sagemath#36676 and sagemath#36951.
    
URL: sagemath#37796
Reported by: roed314
Reviewer(s): Dima Pasechnik
@saraedum
Copy link
Member

This PR is now #37901 which recreates the changes here to revert the revert #37796.

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 p: critical / 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants