Meta-PR: More `# optional` doctest tags #34998

mkoeppe · 2023-02-07T04:18:03Z

We add # optional tags for various doctests, depending on

Sage packages such as sage.symbolic, sage.rings.number_field, sage.groups, sage.libs.singular, sage.modules
standard packages such as pplpy, sympy, numpy (from sage.features: Add sage.libs.singular, features for standard Python packages #35237)

There is no change when doctesting the monolithic sage library, but it eliminates numerous failures when testing modularized distributions such as sagemath-standard-no-symbolics (#35150) and sagemath-polyhedra (#35095).

In short:

Everything that uses factorization needs to be marked sage.libs.pari
Everything that uses linear algebra (including CombinatorialFreeModule) needs to be marked sage.modules
Other annotations are self-explanatory

Depends on:

codecov-commenter · 2023-02-07T20:13:46Z

Codecov Report

Patch coverage has no change and project coverage change: -0.70 ⚠️

Comparison is base (f449b14) 88.62% compared to head (fa9d589) 87.93%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34998      +/-   ##
===========================================
- Coverage    88.62%   87.93%   -0.70%     
===========================================
  Files         2148     2148              
  Lines       398653   398686      +33     
===========================================
- Hits        353308   350571    -2737     
- Misses       45345    48115    +2770

Impacted Files	Coverage Δ
src/sage/arith/misc.py	`90.99% <ø> (ø)`
src/sage/calculus/test_sympy.py	`0.00% <ø> (ø)`
src/sage/categories/additive_magmas.py	`97.77% <ø> (ø)`
src/sage/categories/additive_semigroups.py	`100.00% <ø> (ø)`
src/sage/categories/affine_weyl_groups.py	`100.00% <ø> (ø)`
src/sage/categories/algebra_functor.py	`80.00% <ø> (-20.00%)`	⬇️
src/sage/categories/algebra_ideals.py	`88.88% <ø> (ø)`
src/sage/categories/algebra_modules.py	`100.00% <ø> (ø)`
src/sage/categories/algebras.py	`100.00% <ø> (ø)`
src/sage/categories/algebras_with_basis.py	`96.22% <ø> (ø)`
... and 247 more

... and 83 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mkoeppe · 2023-02-10T06:19:51Z

Tests pass, ready for review

kwankyu · 2023-02-21T12:22:32Z

Tests pass, ready for review

Yes. But to check that # optional - sage.module.... tags are added where necessary, (and remind me this modularization stuff) let me ask some questions:

For instance,

sage: CyclotomicField.__module__
'sage.rings.number_field.number_field'

Then how can I see which is the minimal distribution package that installs the module sage.rings.number_field.number_field?

The minimal distribution package should be unique. Right?

If the smallest distribution package that installs the module containing the doctest using CyclotomicField is not, in the distribution packages hierarchy, above (or the same as) the smallest distribution package that installs the module sage.rings.number_field.number_field, then the doctest should have # optional - sage.rings.number_field tag. Am I right?

Which of sage.rings.number_field and sage.rings.number_field.number_field should be added to # optional - tag?

Is this the only scenario that # optional - sage.module.... needs to be added?

mkoeppe · 2023-02-21T18:59:34Z

Then how can I see which is the minimal distribution package that installs the module sage.rings.number_field.number_field?

Which of sage.rings.number_field and sage.rings.number_field.number_field should be added to # optional - tag?

In src/sage/features/sagemath.py you can see that the actual Python module whose presence is tested for the feature "sage.rings.number_field" is sage.rings.number_field.number_field_element. Often, it is the case that:

the element implementations have a compile-time dependency on a library (in this case, NTL), whereas
the parent implementation may not have such a dependency at compile time or module load time; but
the parent cannot be instantiated if the element implementation module is not present because that involves creating some elements.

"sage.rings.number_field" is the name for the full functionality (not just loading some modules) of the classes defined in sage.rings.number_field.number_field etc.

The minimal distribution package should be unique. Right?

When the modularization takes its final form, every module will be distributed by exactly one distribution package, so uniqueness holds trivially.

However, during the modularization effort, there still exist some distribution packages that are subsets of other distribution packages: Currently, all of sagemath-{environment,repl,objects,categories} are subsets of sagemath-standard. Some PRs introduce additional such cases:

Another PR corrects such subset relations by making the subset distributions dependencies (install-requires) instead:

Refactor sagemath-standard through sagemath-{categories,environment,repl} #35143

During the modularization effort, it may also be convenient to temporarily introduce some nontrivial antichains of distribution packages; in that case, the uniqueness will not necessarily hold. (Such antichains may also be introduced inadvertently by mistakes in writing the MANIFEST.in files.)

If the smallest distribution package that installs the module containing the doctest using CyclotomicField is not, in the distribution packages hierarchy, above (or the same as) the smallest distribution package that installs the module sage.rings.number_field.number_field, then the doctest should have # optional - sage.rings.number_field tag. Am I right?

I think here you are asking about the modules shipped by distribution packages together with their declared runtime dependencies (install-requires).

So the doctest should have # optional - sage.rings.number_field tag whenever the module in which it appears is shipped in a distribution package that does not include the modules that provide the feature "sage.rings.number_field".

kwankyu · 2023-02-21T22:20:48Z

When the modularization takes its final form, every module will be distributed by exactly one distribution package, so uniqueness holds trivially.

There won't be sagemath-standard? There won't be a distribution package that requires a set of smaller distribution packages and includes some individual modules? The hierarchy will disappear?

Or do you mean that every module will be included in exactly one minimal (in subset relations) distribution package?

mkoeppe · 2023-02-21T23:39:40Z

sagemath-standard will continue to exist but most likely will not ship any module. Instead it will declare a bunch of runtime dependencies so that installing it will install all modules.

mkoeppe · 2023-02-21T23:41:58Z

The hierarchy will disappear?

This hierarchy diagram is not intended to visualize inclusion; it visualizes runtime dependencies.

kwankyu · 2023-02-22T00:10:10Z

The hierarchy will disappear?

This hierarchy diagram is not intended to visualize inclusion; it visualizes runtime dependencies.

Suppose there are only three modules A, B, and C.

(1) If distribution package P1 installs modules A and B and distribution package P2 depends on P1 but also installs a module C, then a doctest in C would not need "#optional - A"

(2) If distribution package P1 installs modules A and B and distribution P3 installs the module C and P2 depends on P1 and P3, then the same doctest in C would need "# optional - A"

The modularization in final form would look like (2). Am I right?

mkoeppe · 2023-02-22T01:08:05Z

Suppose there are only three modules A, B, and C.

(1) If distribution package P1 installs modules A and B and distribution package P2 depends on P1 but also installs a module C, then a doctest in C would not need "#optional - A"

That's right. When C is doctested, its unique containing distribution P2 is installed, hence by declared dependencies also P1 is installed, so A is available.

(2) If distribution package P1 installs modules A and B and distribution P3 installs the module C and P2 depends on P1 and P3, then the same doctest in C would need "# optional - A"

Right.

The modularization in final form would look like (2). Am I right?

Both cases can appear in the modularization in final form.

kwankyu · 2023-02-22T02:23:01Z

In the example, the distribution package that installs A is P1. (I added adjective minimal for this above)

In (1), the distribution package that installs C is P2. As P2 is above P1 in the hierarchy, the doctest does not need the tag

In (2), the distribution package that installs C is P3. Here P3 is not above P1, and hence the doctest needs the tag.

So to decide whether the tag is necessary for a doctest, we need to know the distribution that installs a module. This is my first question above.

Is there a command that reports the distribution that installs module M, for input M? Or should we search through the source code of distribution packages?

Is there a command that reports the relationship between two distributions in the hierarchy? Or should we rely on the hierarchy diagram in the developer manual?

kwankyu · 2023-02-22T02:30:55Z

I guess that the hierarchy diagram can be outdated anytime. Then we still need to look through the source code of distribution packages if the command does not exist.

mkoeppe · 2023-02-22T02:38:56Z

So to decide whether the tag is necessary for a doctest, we need to know the distribution that installs a module. [...]
Is there a command that reports the distribution that installs module M, for input M? Or should we search through the source code of distribution packages?

When M is already installed, we can get this information using importlib.metadata.packages_distributions and importlib.metadata.files.
I don't know if there is a more convenient function that gives this information directly.

On the source code level, I hope to add a comment line to the top of each file that indicates the distribution; see #30666, which adds a number of these comments. This will replace the current method of defining the distributions via MANIFEST.in files, which is error-prone (there's no control that we ship every module exactly once).

mkoeppe · 2023-02-22T02:41:49Z

Is there a command that reports the relationship between two distributions in the hierarchy? Or should we rely on the hierarchy diagram in the developer manual?

For installed distributions, importlib.metadata.requires provides this information.

kwankyu · 2023-02-22T04:29:31Z

I wrote quickly this:

from importlib.metadata import packages_distributions, distribution, requires

def sage_distribution_packages():
    sage_dists = []
    for dist_list in packages_distributions().values():
        for dist in dist_list:
            if dist.startswith('sage-') or dist.startswith('sagemath-'):
                sage_dists.append(dist)
    return list(set(sage_dists))

def find_sage_distribution_package(mod):
    dists = []
    for dist_pack in sage_distribution_packages():
        for pack in distribution(dist_pack).files:
            if pack.name.endswith('.py'):
                parts = pack.parts
                mod2 = '.'.join(list(parts[:-1]) + [pack.name[:-3]])
            elif pack.name.endswith('.pyx'):
                parts = pack.parts
                mod2 = '.'.join(list(parts[:-1]) + [pack.name[:-4]])  
            else:
                continue
            if mod2 == mod:
                dists.append(dist_pack)
                break
    return dists

The I get

sage: sage_distribution_packages()
['sagemath-standard', 'sage-docbuild', 'sage-conf']
sage: m = CyclotomicField.__module__; m
'sage.rings.number_field.number_field'
sage: find_sage_distribution_package(m)
['sagemath-standard']
sage: find_sage_distribution_package('sage.structure.coerce')
['sagemath-standard']

So there is no need to add # optional - sage.rings.number_field to the doctest I am looking at in sage.structure.coerce, right now.

But this PR is provisional to modularized distributions sagemath-standard-no-symbolics and sagemath-polyhedra. So this PR depends on other PRs providing the distributions. Right?

mkoeppe · 2023-02-22T04:37:49Z

Thanks for writing this! It would be a useful addition to sage.misc.dev_tools, providing something of the same kind as import_statements

mkoeppe · 2023-02-22T04:39:09Z

Here you ran your code in a non-modularized installation: sagemath-standard ships everything, so your code will tell you that nothing needs a # optional - sage....

mkoeppe · 2023-02-22T04:40:14Z

So this PR depends on other PRs providing the distributions.

No, this PR does not have any dependencies because the added annotations do nothing in the monolithic installation.

kwankyu · 2023-02-22T04:47:47Z

I meant rather that to see the necessity of those tags you added in this PR through the commands, I should run the commands in sage where the new modularized distribution packages are installed.

mkoeppe · 2023-02-22T04:51:25Z

Yes, testing with the subset distributions, for example using make SAGE_CHECK=yes sagemath_categories or (with #32432) make SAGE_CHECK=yes sagemath_polyhedra is the strategy that I used for finding the necessary tags; and it would also be a meaningful strategy for reviewing.

kwankyu · 2023-02-22T09:09:33Z

This merge-conflicts with #35095. Would you make either of them dependent on the other so that they can be reviewed together?

mkoeppe · 2023-02-22T10:02:04Z

Thanks, I've merged it into #35095.

…al_doctest_tags

…tics

…al_doctest_tags

…onal_doctest_tags

…tional_doctest_tags

SageMath version 10.0.rc0, Release Date: 2023-04-23

github-actions · 2023-04-23T21:27:01Z

Documentation preview for this PR is ready! 🎉
Built with commit: ab44e66

…mat doctests    ### 📚 Description  Adding doctest tags `# optional - sage.rings.finite_rings`, `...number_field`, `...padics` etc. for modularization purposes. Also: - some coding style fixes in the doctests (such as adding spaces around some operators and after commas, following PEP 8) - improved the readability of doctests in the HTML format by breaking long lines to avoid having to scroll horizontally    This is: - Part of #29705 - Split out & squashed from #34998 ### 📝 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   URL: #35422 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe

mkoeppe · 2023-07-07T13:27:15Z

Outdated.

mkoeppe added the t: refactoring label Feb 7, 2023

mkoeppe added this to the sage-9.9 milestone Feb 7, 2023

mkoeppe added s: needs review p: minor / 4 labels Feb 7, 2023

dimpase force-pushed the sd117_more_optional_doctest_tags branch from 716c6d7 to 5e7fa7d Compare February 7, 2023 20:12

mkoeppe force-pushed the sd117_more_optional_doctest_tags branch from 5e7fa7d to 7f37354 Compare February 10, 2023 02:21

mkoeppe force-pushed the sd117_more_optional_doctest_tags branch from ad237b3 to ceecd88 Compare February 19, 2023 23:38

mkoeppe requested a review from kliem February 20, 2023 05:28

mkoeppe self-assigned this Feb 21, 2023

Matthias Koeppe added 16 commits April 8, 2023 00:24

sage.rings: Reformat doctests and # optional

4d20f8a

Merge branch 'sage_rings_optional_annotations' into sd117_more_option…

49f068f

…al_doctest_tags

sage.rings: More # optional

2e8a3bb

sage.rings: Reformat doctests and # optional

2d4a62b

src/sage/misc/cachefunc.pyx: Update doctest output

1c15000

src/sage/rings/multi_power_series_ring_element.py: Fix up

c1721ab

sage.rings: Align # optional

a0e7176

Merge branch 'sage_rings_optional_annotations' into sd117_more_option…

82fa637

…al_doctest_tags

sage.rings: Doctest cosmetics

f00b281

sage.rings: More doctest cosmetics

ab389e2

src/sage/rings/integer.pyx: Fix up

1a09248

src/sage/rings/polynomial/multi_polynomial_sequence.py: Doctest cosme…

64d094c

…tics

src/sage/rings/polynomial/multi_polynomial_sequence.py: Fix up

0148f5c

sage.rings: More doctest cosmetics

d7b15d2

src/sage/sets/set_from_iterator.py: Fix up doctest

d8bd93c

Merge branch 'sage_rings_optional_annotations' into sd117_more_option…

4b6a8df

…al_doctest_tags

mkoeppe force-pushed the sd117_more_optional_doctest_tags branch from b55ba8f to 4b6a8df Compare April 9, 2023 20:40

Matthias Koeppe added 2 commits April 9, 2023 13:42

Merge branch 'sage_categories_reformat_doctests' into sd117_more_opti…

c2a9e90

…onal_doctest_tags

Merge branch 'sage_quadratic_forms_modularization' into sd117_more_op…

e340092

…tional_doctest_tags

mkoeppe changed the title ~~More # optional doctest tags~~ Meta-PR: More # optional doctest tags Apr 15, 2023

mkoeppe removed the s: needs review label Apr 15, 2023

Merge tag '10.0.rc0' into sd117_more_optional_doctest_tags

ab44e66

SageMath version 10.0.rc0, Release Date: 2023-04-23

mkoeppe modified the milestones: sage-10.0, sage-10.1 Apr 30, 2023

mkoeppe added the r: duplicate label May 28, 2023

kwankyu mentioned this pull request Jul 7, 2023

Add style guide / reference for # optional - sage.... doctest tags, extend sage -t and sage -fixdoctests for modularization tasks #35749

Merged

5 tasks

mkoeppe closed this Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meta-PR: More `# optional` doctest tags #34998

Meta-PR: More `# optional` doctest tags #34998

mkoeppe commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading

mkoeppe commented Feb 10, 2023

kwankyu commented Feb 21, 2023 •

edited

Loading

mkoeppe commented Feb 21, 2023 •

edited

Loading

kwankyu commented Feb 21, 2023 •

edited

Loading

mkoeppe commented Feb 21, 2023

mkoeppe commented Feb 21, 2023

kwankyu commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023 •

edited

Loading

kwankyu commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023 •

edited

Loading

mkoeppe commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023 •

edited

Loading

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

github-actions bot commented Apr 23, 2023

mkoeppe commented Jul 7, 2023

Meta-PR: More # optional doctest tags #34998

Meta-PR: More # optional doctest tags #34998

Conversation

mkoeppe commented Feb 7, 2023 • edited Loading

codecov-commenter commented Feb 7, 2023 • edited Loading

Codecov Report

mkoeppe commented Feb 10, 2023

kwankyu commented Feb 21, 2023 • edited Loading

mkoeppe commented Feb 21, 2023 • edited Loading

kwankyu commented Feb 21, 2023 • edited Loading

mkoeppe commented Feb 21, 2023

mkoeppe commented Feb 21, 2023

kwankyu commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023 • edited Loading

kwankyu commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023 • edited Loading

mkoeppe commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023 • edited Loading

mkoeppe commented Feb 22, 2023

kwankyu commented Feb 22, 2023

mkoeppe commented Feb 22, 2023

github-actions bot commented Apr 23, 2023

mkoeppe commented Jul 7, 2023

Meta-PR: More `# optional` doctest tags #34998

Meta-PR: More `# optional` doctest tags #34998

mkoeppe commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading

kwankyu commented Feb 21, 2023 •

edited

Loading

mkoeppe commented Feb 21, 2023 •

edited

Loading

kwankyu commented Feb 21, 2023 •

edited

Loading

kwankyu commented Feb 22, 2023 •

edited

Loading

kwankyu commented Feb 22, 2023 •

edited

Loading

kwankyu commented Feb 22, 2023 •

edited

Loading