-
-
Notifications
You must be signed in to change notification settings - Fork 484
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
Restructure sage.*.all
for modularization, replace relative by absolute imports
#36676
Conversation
a9e5ea8
to
79b1797
Compare
sage.*.all
for modularizationsage.*.all
for modularization, replace relative by absolute imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coding style can certainly be improved and the syntax used when importing multiple methods from a module could be unified.
I tried on Fedora 35 and it seems ok.
I would be in favor of running tools like autopep8, ruff, or black on these files to implement a consistent style. I don't have a personal preference for any particular style, nor do I regularly use these tools for auto-formatting. So discussion, suggestion, help are all welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like all these all
files. They add a lot of noise without providing much added value. I thought a bit about alternatives and came up with the following idea:
- Each distribution sets a global constant (say
IS_CATEGORIES_DISTRO
for the categories distro) - In the single all file, you then have blocks that conditionally import certain modules depending on these global flag.
In this way you can introduce as many distributions as you want without having many all files. This is similar to the TYPE_CHECKING
flag used by pyright.
What do you think?
from sage.misc.lazy_import import lazy_import | ||
lazy_import('sage.algebras.steenrod.steenrod_algebra', ['SteenrodAlgebra', 'Sq']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why convert this to a lazy import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was convenient in the modularization. Do you have a specific concern about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For another ticket: I think that SteenrodAlgebra
(which I wrote) and other named algebras perhaps could be deprecated and accessed solely through the algebras catalog, as a way of cleaning up the namespace. (I have no concerns about lazy importing here, for what that's worth.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was convenient in the modularization. Do you have a specific concern about it?
Just that it makes it very hard to review this PR, as there are all these unrelated changes sprinkled in. Can you extract these lazy_import changes to a new PR please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't believe this would be helpful for anything.
src/sage/categories/all.py
Outdated
@@ -27,128 +27,128 @@ | |||
from sage.misc.namespace_package import install_doc | |||
install_doc(__package__, __doc__) | |||
|
|||
from . import primer | |||
from sage.categories import primer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please revert these changes to modules that you don't touch otherwise. Just increases the diff unnecessarily and introduces merge conflicts with other PRs. E.g. these changes here are already included in #36572.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the PR is to replace all the all files with the version provided here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introduces merge conflicts with other PRs. E.g. these changes here are already included in #36572.
When the same change appears, git automatically resolves it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the PR is to replace all the all files with the version provided here.
How is this change relevant to the modularization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is part of "replace relative by absolute imports", which is in the title of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: The PR mentioned above, #36572, was merged in develop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still no logical connection between making the imports absolute and the modularization. Both touch the all files yes, but that doesn't mean they should be in the same PR.
This idea does not work because after an |
In particular: https://stackoverflow.com/questions/76771858/ruff-does-not-autofix-line-too-long-violation So there is a choice between the style that diff --git a/src/sage/schemes/hyperelliptic_curves/all.py b/src/sage/schemes/hyperelliptic_curves/all.py
index e314dceb1e0..40c8b58e823 100644
--- a/src/sage/schemes/hyperelliptic_curves/all.py
+++ b/src/sage/schemes/hyperelliptic_curves/all.py
@@ -41,6 +41,7 @@ lazy_import('sage.schemes.hyperelliptic_curves.invariants',
['igusa_clebsch_invariants', 'absolute_igusa_invariants_kohel',
'absolute_igusa_invariants_wamelen', 'clebsch_invariants'],
deprecation=28064)
-from sage.schemes.hyperelliptic_curves.mestre import (Mestre_conic, HyperellipticCurve_from_invariants)
+from sage.schemes.hyperelliptic_curves.mestre import (
+ Mestre_conic, HyperellipticCurve_from_invariants)
from sage.schemes.hyperelliptic_curves import monsky_washnitzer
del lazy_import
diff --git a/src/sage/stats/hmm/all.py b/src/sage/stats/hmm/all.py
index d331f45b41c..2d88d943fb1 100644
--- a/src/sage/stats/hmm/all.py
+++ b/src/sage/stats/hmm/all.py
@@ -8,6 +8,7 @@
# We lazy_import the following modules since they import numpy which slows down sage startup
from sage.misc.lazy_import import lazy_import
lazy_import("sage.stats.hmm.hmm", ["DiscreteHiddenMarkovModel"])
-lazy_import("sage.stats.hmm.chmm", ["GaussianHiddenMarkovModel","GaussianMixtureHiddenMarkovModel"])
+lazy_import("sage.stats.hmm.chmm", [
+ "GaussianHiddenMarkovModel","GaussianMixtureHiddenMarkovModel"])
lazy_import("sage.stats.hmm.distributions", ["GaussianMixtureDistribution"])
del lazy_import and what --- a/src/sage/schemes/hyperelliptic_curves/all.py
+++ b/src/sage/schemes/hyperelliptic_curves/all.py
@@ -37,10 +37,21 @@ from sage.misc.lazy_import import lazy_import
from sage.schemes.hyperelliptic_curves.constructor import HyperellipticCurve
from sage.schemes.hyperelliptic_curves.kummer_surface import KummerSurface
-lazy_import('sage.schemes.hyperelliptic_curves.invariants',
- ['igusa_clebsch_invariants', 'absolute_igusa_invariants_kohel',
- 'absolute_igusa_invariants_wamelen', 'clebsch_invariants'],
- deprecation=28064)
-from sage.schemes.hyperelliptic_curves.mestre import (Mestre_conic, HyperellipticCurve_from_invariants)
+
+lazy_import(
+ "sage.schemes.hyperelliptic_curves.invariants",
+ [
+ "igusa_clebsch_invariants",
+ "absolute_igusa_invariants_kohel",
+ "absolute_igusa_invariants_wamelen",
+ "clebsch_invariants",
+ ],
+ deprecation=28064,
+)
+from sage.schemes.hyperelliptic_curves.mestre import (
+ Mestre_conic,
+ HyperellipticCurve_from_invariants,
+)
from sage.schemes.hyperelliptic_curves import monsky_washnitzer
+
del lazy_import
diff --git a/src/sage/stats/hmm/all.py b/src/sage/stats/hmm/all.py
index d331f45b41c..6c879e12f5d 100644
--- a/src/sage/stats/hmm/all.py
+++ b/src/sage/stats/hmm/all.py
@@ -7,7 +7,11 @@
# We lazy_import the following modules since they import numpy which slows down sage startup
from sage.misc.lazy_import import lazy_import
+
lazy_import("sage.stats.hmm.hmm", ["DiscreteHiddenMarkovModel"])
-lazy_import("sage.stats.hmm.chmm", ["GaussianHiddenMarkovModel","GaussianMixtureHiddenMarkovModel"])
+lazy_import(
+ "sage.stats.hmm.chmm",
+ ["GaussianHiddenMarkovModel", "GaussianMixtureHiddenMarkovModel"],
+)
lazy_import("sage.stats.hmm.distributions", ["GaussianMixtureDistribution"])
del lazy_import |
Can you explain why one would import the all files multiple times? Is it not enough to import it once, determine which distros are installed and then import the correct submodules based on this info? |
No, the main purpose of the "all" files is to fill the top-level namespace. But the contents of a module such as Instead I define separate top-levels that are suitable for the separate testing of the distributions. (I've added a link on that to the ticket description.) Edit: As I wrote in #36524 (comment), this is part of the solution of how to test things when not all of |
The documentation don't build due to
|
Why do you think this would be a problematic design? With your version you also have to determine which distros are installed and then import the corresponding all file. |
No, that's not what is done. Read the documentation link I added to the ticket description. |
883e05f
to
e349b00
Compare
88b91b5
to
fb162de
Compare
I went with autopep8. |
0f5ef2f
to
d62bed8
Compare
I think I've fixed it now in 94cca3f |
I am now casting a vote of -1 on this PR. The reason is that I do not properly understan the implications of the annotations of the form I think that separating math into classes like |
I think that separating math into classes like graph-theory, combinatorics,
symbolics in a computer algebra system looks very strange and is bound to
lead to problems in the long run, but I may be convinced otherwise.
My understanding is that it's more about what other software is needed for
the different modules (e.g., some users might not want to install Lisp to
do symbolic integration via Maxima). So while there is a correlation
loosely to mathematical specialties, I guess it doesn't have to be strict.
… Message ID: ***@***.***>
|
I'm afraid I now have more questions than before, please bear with me :-) 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? 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. 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 - Also, I find it hard to believe that it is about dependencies, because the stuff in |
I think that separating math into classes like graph-theory, combinatorics,
symbolics in a computer algebra system looks very strange and is bound to
lead to problems in the long run, but I may be convinced otherwise.
My understanding is that it's more about what other software is needed for
the different modules (e.g., some users might not want to install Lisp to
do symbolic integration via Maxima). So while there is a correlation
loosely to mathematical specialties, I guess it doesn't have to be strict.
I'm afraid I now have more questions than before, please bear with me :-)
Sorry, I've already exhausted my knowledge :-) but hopefully Matthias or
others can clarify your good questions.
… Message ID: ***@***.***>
|
Some are. See the required reading. Links in #29705. |
@mantepse Don't fear. This has been thought through carefully, and you can read about it in the Sage developer's guide. |
The chain of immediate ancestors of this commit was created by cherry-picking the relevant commits that comprise sagemath#36676. Replaying all these commits (and the necessary conflict resolution) still leaves a non-empty diff when comparing 10.4.beta2 to the previous sagemath#36676 and 10.4.beta3 to that cherry-picked branch. This commit makes sure that the diff is trivial. The changes introduced here were likely introduced in conflict resolution when merging develop into sagemath#36676. I did not replay these conflict resolutions, so I do this here manually.
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
I think Sage should move forward with the modularization project. I vote +1 on this PR. |
+1 |
I posed more precise versions of the questions above in #36964 (comment). Since they have not been answered yet, and that pull request is already closed, I'll repeat them here. 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
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 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 - Also, I find it hard to believe that it is about dependencies, because the stuff in |
The SnapPy project is one example. This is a stand-alone Python package which gains extra functionality when used within Sage. I would much prefer we just required the (relevant parts of) the Sage math library, but we want SnapPy to be part of the broader Python ecosystem, being available on PyPI and installable via More broadly, I strongly agree with William Stein that Sage needs to be part of the broader Python ecosystem. To me, that's the main purpose of the modularization project, to fit it in better with the rest of the Scientific Python community. The correct level of granularity to choose for modularization is not an easy technical question, but I believe some level of modularization (e.g. making Lisp optional) is necessary to achieve this goal. |
Yes, I agree completely. However, this means we want to distinguish between non-python dependencies, and not between subfields of mathematics, and the current pull request does not do this, as far as I can tell. I think I would be quite OK with having, for example, |
Thanks @culler and @GMS103 for the support. This brings the tally to:
|
That should not happen. Modularization is downstream to the sage library. Yes, we are restructuring some parts of the sage library to fit with modularization. But modularization should never be an obstacle in developing the sage library. If it ever be, the sage community might drop the modularization project. |
That one, more or less, is the distribution package sagemath-standard-no-symbolics. |
@NathanDunfield Thanks for sharing this clear example. Would you be able to estimate how much work (if any) has been necessary to make Sage an optional dependency only? (In our June 2023 discussion in https://groups.google.com/g/sage-devel/c/lb1Eq8o0a5I/m/JJQvmPzfAQAJ, you indicated "Right now, in the stand-alone apps we backstop some of Sage's functionality (e.g. arbitrary precision floats) with CyPari".) |
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
Closing as this PR has been replaced by / resurrected as #37900. |
….all` <!-- ^ 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". --> Cherry-picked from sagemath#36676/sagemath#37900. ### 📝 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 and checked the documentation preview. ### ⌛ 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#37993 Reported by: Matthias Köppe Reviewer(s): John H. Palmieri, Matthias Köppe
….all` <!-- ^ 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". --> Cherry-picked from sagemath#36676/sagemath#37900. ### 📝 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 and checked the documentation preview. ### ⌛ 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#37993 Reported by: Matthias Köppe Reviewer(s): John H. Palmieri, Matthias Köppe
….all` <!-- ^ 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". --> Cherry-picked from sagemath#36676/sagemath#37900. ### 📝 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 and checked the documentation preview. ### ⌛ 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#37993 Reported by: Matthias Köppe Reviewer(s): John H. Palmieri, Matthias Köppe
….all` <!-- ^ 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". --> Cherry-picked from sagemath#36676/sagemath#37900. ### 📝 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 and checked the documentation preview. ### ⌛ 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#37993 Reported by: Matthias Köppe Reviewer(s): John H. Palmieri, Matthias Köppe
We restructure the
all.py
files for modularization.Guided by the technical constraints outlined in https://groups.google.com/g/sage-devel/c/ozh-7ZZ848s, #35095 defines distribution packages (pip-installable packages) sagemath-{brial, combinat, eclib, flint, gap, giac, glpk, graphs, groups, homfly, lcalc, libbraiding, libecm, linbox, modules, mpmath, ntl, pari, plot, polyhedra, schemes, singular, standard-no-symbolics, symbolics}.
When a namespace package such as
sage.misc
is filled by modules from several distribution packages, we create modules named:src/sage/misc/all__sagemath_environment.py
src/sage/misc/all__sagemath_objects.py
src/sage/misc/all__sagemath_repl.py
Import statements are moved from
src/sage/misc/all.py
to these files as appropriate, andsrc/sage/misc/all.py
imports*
from there.Also some imports are replaced by lazy imports.
The new files provide the top level namespaces for the modularized distribution packages, thus enabling modularized testing.
This design was introduced in #29865 (merged in Jan 2022, early in the Sage 9.6 development cycle).
Moreover, applied a one-line command to replace relative by absolute imports, thus complementing #36666, which does not touch
.all*
files.The changes to other files in
sage.modules
etc. come from the PRs #36597, #36572, #36588, #36589 merged here and do not need review.📝 Checklist
⌛ Dependencies