`sage.{modular,schemes}`: Modularization fixes for imports; update `sage -fiximports`, add relint pattern by mkoeppe · Pull Request #35884 · sagemath/sage

mkoeppe · 2023-07-02T19:25:45Z

📚 Description

📝 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

Depends on src/sage/schemes/projective/proj_bdd_height.py: Fix markup #35974 (to soothe the linter)

… imports

…up imports

SageMath version 10.1.beta7, Release Date: 2023-07-20

mkoeppe · 2023-07-21T20:35:03Z

The Linter failure is unrelated and fixed in #35974

kwankyu · 2023-07-21T20:46:20Z

src/sage/schemes/elliptic_curves/padic_lseries.py


-import sage.matrix.all as matrix
+from sage.matrix.constructor import Matrix as matrix
 import sage.schemes.hyperelliptic_curves.monsky_washnitzer


why not just from sage.matrix.constructor import matrix?

OK, done (in all places)

src/sage/schemes/elliptic_curves/ell_tate_curve.py

kwankyu · 2023-07-21T21:24:27Z

src/sage/modular/pollack_stevens/padic_lseries.py

 from sage.structure.sage_object import SageObject

+lazy_import('sage.rings.padics.factory', 'Qp', as_='pAdicField')
+


Why only this is lazy-imported while PrecisionError from sage.rings.padics.precision_error is just imported?

sage.rings.padics.precision_error is shipped by sagemath-categories

One cannot do try: ... except ... with a LazyImport

kwankyu · 2023-07-21T21:27:11Z

src/sage/modular/modsym/space.py


-from . import hecke_operator
+lazy_import('sage.modular.modsym', 'hecke_operator')



Why is this lazy-imported?

IIRC, via .hecke it depends on .arithgroup, which I had some difficulties with

kwankyu · 2023-07-21T21:30:07Z

src/sage/modular/modsym/all.py


-from .heilbronn import HeilbronnCremona, HeilbronnMerel
+lazy_import('sage.modular.modsym.heilbronn', ['HeilbronnCremona', 'HeilbronnMerel'])



How do you decide that this should be lazy-imported?

It has a compile-time dependency on FLINT and won't be shipped in sagemath-schemes

You meant sagemath-modular?

…xargs sed -i.bak 's/from sage.matrix.constructor import Matrix as matrix/from sage.matrix.constructor import matrix/'

src/.relint.yml

kwankyu · 2023-07-22T14:02:36Z

src/.relint.yml

+  pattern: '^import\s+sage(|[.](combinat|crypto|databases|data_structures|dynamics|ext|game_theory|games|geometry|graphs|groups|interfaces|manifolds|matrix|matroids|misc|modules|monoids|numerical|probability|quadratic_forms|quivers|rings|sat|schemes|sets|stats|symbolic|tensor)[a-z0-9_.]*|[.]libs)[.]all'
+  # imports from .all are allowed in all.py; also allow in some modules that need sage.all
+  filePattern: '(.*/|)(?!(all|benchmark|dev_tools|parsing|sage_eval|explain_pickle|.*_test))[^/.]*[.](py|pyx|pxi)$'
+


You don't add explain_pickle also in L54?

OK, done in 5a6d6bd

kwankyu · 2023-07-22T14:08:11Z

src/sage/misc/replace_dot_all.py


-def walkdir_replace_dot_all(dir, file_regex=r'.*[.](py|pyx|pxi)$', package_regex=None, verbose=False):
+excluded_file_regex = 'auto-methods|replace_dot_all'
+


You may want r' like below

Do we actually need L388? The argument excluded_file_regex of walkdir_replace_dot_all gets the default value. No?

Thanks. I've removed it in 299ae87.

kwankyu · 2023-07-22T14:38:11Z

To justify fixes of modularization-anti-pattern imports, I think each file should be annotated with the distribution which the file belongs to (is installed by). Do we already have a standard style for the annotation?

For file.py, the annotated distribution would be used as xxx in sage -t --distribution=xxx file.py. Right?

I think that the future incremental test workflow should run sage -t --distribution=xxx file.py for each modified file.py. Right?

kwankyu · 2023-07-22T15:11:07Z

As more files of the sage library get included to newly born distributions, the work practice of a sage developer should change much, albeit there has been little change in the practice up to now. A developer would modify a file such that the file does not break in the distribution it belongs to. If doctests that need other distributions are added, then he needs to test the doctests in another sage install where the additional distributions are available as well.

If the above is right, perhaps we should also update the developer manual to reflect the changing work practice.

In the worst, a developer would feel like that he/she is expelled from their paradise (where one can import anything from anywhere), after they were scared by the huge flood of optional tags to doctests.

…uded_file_regex

mkoeppe · 2023-07-22T17:53:05Z

I think each file should be annotated with the distribution which the file belongs to (is installed by). Do we already have a standard style for the annotation?

Yes, that's # sage_setup: distribution = sagemath-bliss, which is currently only used for the few modules depending on optional packages. #35663 is adding many more such directives.

mkoeppe · 2023-07-22T17:58:04Z

For file.py, the annotated distribution would be used as xxx in sage -t --distribution=xxx file.py. Right?

Yes, that's for sure one of the tests to run, but sometimes one also may want to test with extras added to enable more tests.

I think that the future incremental test workflow should run sage -t --distribution=xxx file.py for each modified file.py.

Right.

mkoeppe · 2023-07-22T18:31:05Z

As more files of the sage library get included to newly born distributions, the work practice of a sage developer should change much, albeit there has been little change in the practice up to now. A developer would modify a file such that the file does not break in the distribution it belongs to. If doctests that need other distributions are added, then he needs to test the doctests in another sage install where the additional distributions are available as well.

If the above is right, perhaps we should also update the developer manual to reflect the changing work practice.

Yes, that's a great point. While #35749 is adding a detailed description of how to test in a modularized setting, something has yet to be added to the tutorials that describe the development workflow.

In the worst, a developer would feel like that he/she is expelled from their paradise (where one can import anything from anywhere), after they were scared by the huge flood of optional tags to doctests.

There is certainly a potential for developer frustration after we make failures of the modularized distributions hard errors. This is something to watch out for and mitigate.

kwankyu · 2023-07-22T19:42:54Z

I think each file should be annotated with the distribution which the file belongs to (is installed by). Do we already have a standard style for the annotation?

Yes, that's # sage_setup: distribution = sagemath-bliss, which is currently only used for the few modules depending on optional packages. #35663 is adding many more such directives.

Certainly adding those annotations is a huge task and error-prone, and it is good that there is a one source of truth. That one source of truth should be MANIFEST.in.

Hence it would be nice that we can add the annotations automatically from the manifest files, perhaps as part of the bootstrap. I asked chatGPT how to do that. It answered

python setup.py sdist --manifest-only

But this does not work with me. Its second answer is

python setup.py check --metadata --restructuredtext --strict && python setup.py sdist --formats=zip,gztar

This seems to work. I ran the command in pkgs/sagemath-objects, and got the file pkgs/sagemath-objects/sagemath_objects.egg-info/SOURCES.txt, which list the included files. We may use this file to add the annotations automatically.

Does this make sense?

mkoeppe · 2023-07-22T21:18:10Z

that's # sage_setup: distribution = sagemath-bliss, which is currently only used for the few modules depending on optional packages. #35663 is adding many more such directives.

Certainly adding those annotations is a huge task and error-prone, and it is good that there is a one source of truth. That one source of truth should be MANIFEST.in.

Right now, this is true to some extent, but it's a bit more complicated actually. Some of the setup.py files also do source filtering using the # sage_setup: distribution directives, in addition to the filtering that MANIFEST does.

I'm hoping to unify and simplify this soon; not sure if I really want to keep the MANIFEST mechanism.
Using the # sage_setup: distribution directives as the one source of truth would have the benefit that distributions are guaranteed to be disjoint.

Hence it would be nice that we can add the annotations automatically from the manifest files, perhaps as part of the bootstrap.

This can't be part of the bootstrap because it would need to modify source files that are under version control.

But it would be indeed be very useful as a developer tool when making changes to the contents of the distributions. I'm currently doing this with very primitive tools.

mkoeppe · 2023-07-22T21:22:52Z

I ran the command in pkgs/sagemath-objects, and got the file pkgs/sagemath-objects/sagemath_objects.egg-info/SOURCES.txt, which list the included files. We may use this file to add the annotations automatically.

A better command is ./sage -sh -c '(cd pkgs/sagemath-objects && python3 setup.py egg_info)'

kwankyu · 2023-07-23T00:09:21Z

Certainly adding those annotations is a huge task and error-prone, and it is good that there is a one source of truth. That one source of truth should be MANIFEST.in.

Right now, this is true to some extent, but it's a bit more complicated actually. Some of the setup.py files also do source filtering using the # sage_setup: distribution directives, in addition to the filtering that MANIFEST does.

I'm hoping to unify and simplify this soon; not sure if I really want to keep the MANIFEST mechanism. Using the # sage_setup: distribution directives as the one source of truth would have the benefit that distributions are guaranteed to be disjoint.

We may use MANIFEST (or something renamed) as a tool to mass-add the file annotations, but use the file annotations as the one source of truth.

From developer's point of view, I think both mechanisms would be useful.

kwankyu · 2023-07-23T00:50:44Z

src/sage/modular/hecke/ambient_module.py

-import sage.rings.all
+from sage.modules.free_module import FreeModule, is_FreeModule
+from sage.rings.integer import Integer



Couldn't we get rid of is_FreeModule on the way?

Perhaps not in this PR...

Right, separate PR

kwankyu · 2023-07-23T01:01:38Z

Please soothe the linter. Otherwise, looks good to me.

…ization

kwankyu

Thanks.

mkoeppe · 2023-07-23T04:30:02Z

Thank you!

vbraun · 2023-07-26T21:15:49Z

**********************************************************************
File "src/sage/schemes/elliptic_curves/hom.py", line 1163, in sage.schemes.elliptic_curves.hom.compute_trace_generic
Failed example:
    compute_trace_generic(-m7)
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.elliptic_curves.hom.compute_trace_generic[24]>", line 1, in <module>
        compute_trace_generic(-m7)
      File "/home/release/Sage/src/sage/schemes/elliptic_curves/hom.py", line 1191, in compute_trace_generic
        P = point_of_order(E, l)
            ^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/schemes/elliptic_curves/ell_field.py", line 2083, in point_of_order
        l = rings.ZZ(l)
            ^^^^^
    NameError: name 'rings' is not defined
**********************************************************************
2 items had failures:
   2 of  10 in sage.schemes.elliptic_curves.hom.EllipticCurveHom.characteristic_polynomial
   5 of  26 in sage.schemes.elliptic_curves.hom.compute_trace_generic
    [282 tests, 7 failures, 1.27 s]
----------------------------------------------------------------------
sage -t --long --warn-long 35.9 --random-seed=123 src/sage/schemes/elliptic_curves/hom.py  # 7 doctests failed
----------------------------------------------------------------------

SageMath version 10.1.beta8, Release Date: 2023-07-30

SageMath version 10.1.beta9, Release Date: 2023-08-05

github-actions · 2023-08-06T01:58:27Z

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

sagemathgh-35884: `sage.{modular,schemes}`: Modularization fixes for imports; update `sage -fiximports`, add relint pattern    ### 📚 Description    - Part of: sagemath#29705 - Cherry-picked from: sagemath#35095  ### 📝 Checklist  - [x] 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  - Depends on sagemath#35974 (to soothe the linter)  URL: sagemath#35884 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe

sagemathgh-36135: `sage -fixdistributions`   This is a new maintenance command for adding/updating `# sage_setup: distribution` directives at the top of source files. Based on a discussion in sagemath#35884 (comment)   - Part of: sagemath#29705 - Cherry-picked from: sagemath#35095  ### 📝 Checklist     - [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  - Depends on sagemath#36533 (CI fix)  URL: sagemath#36135 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe

Matthias Koeppe added 19 commits July 2, 2023 12:21

sage.modular: Modularization fixes for imports

442deb7

src/.relint.yml: Flag another type of .all import

3313b79

sage -fiximports: Exclude an autogenerated file

98073c7

sage.schemes: Modularization fixes for imports

62cdde8

src/sage/schemes/elliptic_curves/ell_point.py: Modularization fix for…

785ba39

… imports

src/.relint.yml (namespace_pkg_all_import_2): Exclude explain_pickle

e2778a7

src/sage/lfunctions/lcalc.py: Modularization fixes

4b42cca

src/sage/schemes/elliptic_curves/ell_rational_field.py: Fix more imports

711376f

Fix up import

0645cb2

src/sage/schemes/elliptic_curves/constructor.py: Fix up

0ccf195

sage.modular: Modularization fixes for imports

04e515b

src/sage/modular/dirichlet.py: Fix import

555b2fd

src/sage/modular/modform_hecketriangle/abstract_space.py: Fix up import

b980bc0

src/sage/modular/modform_hecketriangle/hecke_triangle_groups.py: Fix …

3dcafb7

…up imports

src/sage/modular/modform_hecketriangle/graded_ring_element.py: Fix up

d5508df

src/sage/modular/abvar/finite_subgroup.py: Add missing import

fdb6c66

sage.modular: Work around lazy_import limitations

999d106

Merge branch 'develop' into sage_modular_modularization

83c9b27

Merge tag '10.1.beta7' into sage_modular_modularization

4c655bd

SageMath version 10.1.beta7, Release Date: 2023-07-20

kwankyu reviewed Jul 21, 2023

View reviewed changes

src/sage/schemes/elliptic_curves/ell_tate_curve.py Show resolved Hide resolved

kwankyu reviewed Jul 21, 2023

View reviewed changes

git grep -l 'from sage.matrix.constructor import Matrix as matrix' | …

1fe5d38

…xargs sed -i.bak 's/from sage.matrix.constructor import Matrix as matrix/from sage.matrix.constructor import matrix/'

kwankyu reviewed Jul 22, 2023

View reviewed changes

src/.relint.yml Show resolved Hide resolved

kwankyu reviewed Jul 22, 2023

View reviewed changes

Matthias Koeppe added 2 commits July 22, 2023 10:43

src/.relint.yml: Use the same filePattern for namespace_pkg_all_import*

5a6d6bd

src/sage/misc/replace_dot_all.py: Remove redundant definition of excl…

299ae87

…uded_file_regex

kwankyu reviewed Jul 23, 2023

View reviewed changes

Merge branch 'proj_bdd_height_markup_fixes' into sage_modular_modular…

cb2be8e

…ization

kwankyu approved these changes Jul 23, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Jul 23, 2023

vbraun added the s: needs work label Jul 26, 2023

Matthias Koeppe added 3 commits July 30, 2023 12:31

Merge tag '10.1.beta8' into sage_modular_modularization

d511be0

SageMath version 10.1.beta8, Release Date: 2023-07-30

Merge branch 'ci_build_fix_optional' into sage_modular_modularization

7d5d300

Merge tag '10.1.beta9' into sage_modular_modularization

bcd1d6e

SageMath version 10.1.beta9, Release Date: 2023-08-05

vbraun merged commit d5a3fd1 into sagemath:develop Aug 13, 2023

github-actions bot removed the s: positive review label Aug 13, 2023

		from sage.structure.sage_object import SageObject

		lazy_import('sage.rings.padics.factory', 'Qp', as_='pAdicField')


		from . import hecke_operator
		lazy_import('sage.modular.modsym', 'hecke_operator')


		from .heilbronn import HeilbronnCremona, HeilbronnMerel
		lazy_import('sage.modular.modsym.heilbronn', ['HeilbronnCremona', 'HeilbronnMerel'])


		def walkdir_replace_dot_all(dir, file_regex=r'.*[.](py\|pyx\|pxi)$', package_regex=None, verbose=False):
		excluded_file_regex = 'auto-methods\|replace_dot_all'

Uh oh!

Conversation

mkoeppe commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Description

📝 Checklist

⌛ Dependencies

Uh oh!

mkoeppe commented Jul 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwankyu commented Jul 22, 2023

Uh oh!

kwankyu commented Jul 22, 2023

Uh oh!

mkoeppe commented Jul 22, 2023

Uh oh!

mkoeppe commented Jul 22, 2023

Uh oh!

mkoeppe commented Jul 22, 2023

Uh oh!

kwankyu commented Jul 22, 2023

Uh oh!

mkoeppe commented Jul 22, 2023

Uh oh!

mkoeppe commented Jul 22, 2023

Uh oh!

kwankyu commented Jul 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwankyu commented Jul 23, 2023

Uh oh!

kwankyu left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Jul 23, 2023

Uh oh!

vbraun commented Jul 26, 2023

Uh oh!

github-actions bot commented Aug 6, 2023

Uh oh!

Reviewers

Assignees

Labels

mkoeppe commented Jul 2, 2023 •

edited

Loading