Eliminate use of init.py files for supplying package docstrings #32508

mkoeppe · 2021-09-12T06:32:02Z

... as in src/sage/categories/__init__.py or src/sage/combinat/__init__.py

(part of #32501)

CC: @tscrim @kwankyu @nthiery

Component: refactoring

Author: Kwankyu Lee

Branch: ee4367d

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/32508

The text was updated successfully, but these errors were encountered:

kwankyu · 2021-10-28T08:57:38Z

Branch: u/klee/32508

kwankyu · 2021-10-28T08:57:38Z

Author: Kwankyu Lee

sagetrac-git · 2021-10-28T08:57:40Z

Commit: 5dcb3f5

sagetrac-git · 2021-10-28T08:57:40Z

Branch pushed to git repo; I updated commit sha1. New commits:

`5dcb3f5`	`Remove docstrings from __init__.py`

kwankyu · 2021-10-28T09:03:17Z

comment:4

Previously

sage: import sage.combinat
sage: sage.combinat.quickref?

worked. Not anymore with the patch. We should

sage: import sage.combinat.quickref
sage: sage.combinat.quickref?

The same with sage.combinat.tutorial

kwankyu · 2021-10-29T06:50:52Z

comment:5

More seriously, before the patch,

sage: import sage.combinat
sage: help(sage.combinat)

gives a nice ... help. Could we retain this functionality somehow while eliminating the content of sage.combinat.__init__.py?

mkoeppe · 2021-10-29T07:01:17Z

comment:6

I guess with a bit of extra code in src/sage/misc/sagedoc.py, where help is defined, we could do this

kwankyu · 2021-10-29T07:08:46Z

comment:7

Replying to @mkoeppe:

I guess with a bit of extra code in src/sage/misc/sagedoc.py, where help is defined, we could do this

I cannot guess how. Where should the content go? Do you imagine a file like src/sage/combinat/doc.rst or something?

kwankyu · 2021-10-29T10:44:32Z

comment:8

src/sage/combinat/__doc__.py would be better name for that.

mkoeppe · 2021-10-29T16:56:38Z

comment:9

+1 on __doc__.

kwankyu · 2021-10-30T03:43:51Z

comment:10

Replying to @mkoeppe:

+1 on __doc__.

Do you mean __doc__ without .py extension?

mkoeppe · 2021-10-30T03:53:41Z

comment:11

With .py, like you suggested

kwankyu · 2021-10-30T03:54:31Z

comment:12

Then __doc__.py with a python docstring ("""...""")?

kwankyu · 2021-10-30T03:58:57Z

comment:13

On the other hand, I wonder if there is a solution(or recommendation) for this problem(package docstring) in python level as this seems a general issue with namespace packages.

mkoeppe · 2021-10-30T04:17:20Z

comment:14

Replying to @kwankyu:

Then __doc__.py with a python docstring ("""...""")?

Yes, that's what I thought

mkoeppe · 2021-10-30T04:18:14Z

comment:15

Replying to @kwankyu:

On the other hand, I wonder if there is a solution(or recommendation) for this problem(package docstring) in python level as this seems a general issue with namespace packages.

I agree, it would be good to find out whether there are existing practices for this in the Python community.

kwankyu · 2021-11-01T00:08:25Z

comment:16

Replying to @mkoeppe:

Replying to @kwankyu:

Then __doc__.py with a python docstring ("""...""")?

Yes, that's what I thought

One problem with __doc__.py is that it is a fake module for it cannot be imported.

How about this:

src/sage/combinat/__init__/__init__.py

Then the namespace package sage.combinat get to have a subpackage __init__ initialized by __init__.py, which we perhaps may use for "some initialization" as well as for package docstring.

Is this too much or incompatible with the modularization plan?

kwankyu · 2021-11-01T00:09:51Z

comment:17

Replying to @mkoeppe:

I agree, it would be good to find out whether there are existing practices for this in the Python community.

I googled for it, but found none.

mkoeppe · 2021-11-01T01:24:47Z

comment:18

How about we just use all.py for this purpose?

(Modularized packages such as sagemath-polyhedra (#32432) add files such as all__sagemath_polyhedra.py)

kwankyu · 2021-11-01T01:55:35Z

comment:19

Replying to @mkoeppe:

How about we just use all.py for this purpose?

(Modularized packages such as sagemath-polyhedra (#32432) add files such as all__sagemath_polyhedra.py)

How does all.py work?

(1) Does it exist by itself and import other all__xxx files into it?

(2) Or is it dynamically created by combining all__xxx files?

Anyway, if it is at src/sage/combinat/all.py, then it cannot be imported. Can it be?

mkoeppe · 2021-11-01T02:03:01Z

comment:20

Yes, all.py exists as a file in Sage. For namespace packages, I am modularizing it by splitting parts of the existing all.py out to all__sagemath_objects.py, all__sagemath_categories.py, all__sagemath_polyhedra etc. When a distribution has another distribution as a dependency, then it imports the dependency's all__... module. For example, sagemath-polyhedra has sagemath-categories as a dependency, so you see the imports: https://github.com/sagemath/sagetrac-mirror/blob/a58ea1f6decbb84102bd341f8b3cc20f3147452b/src/sage/all__sagemath_polyhedra.py&id2=f716a0b366e31bbb546230140489244cfb68390d and https://github.com/sagemath/sagetrac-mirror/blob/a58ea1f6decbb84102bd341f8b3cc20f3147452b/src/sage/rings/all__sagemath_polyhedra.py&id2=f716a0b366e31bbb546230140489244cfb68390d

mkoeppe · 2021-11-01T02:07:53Z

comment:21

Generally, if the all module is present, then it can also be imported.
Likewise, if an all__... module is present, then it can also be imported.

mkoeppe · 2021-11-01T02:08:50Z

comment:22

If sage.misc.sagedoc.help is used on a package P without __doc__ attribute, it could try to import everything in dir(P) whose name begins with all and accumulate the docstrings of the importable modules.

kwankyu · 2021-11-01T02:36:31Z

comment:23

Replying to @mkoeppe:

Generally, if the all module is present, then it can also be imported.
Likewise, if an all__... module is present, then it can also be imported.

Hmm. if the all module is just below in a namespace package (without __init__.py), I cannot import it. Do I misunderstand something?

For experiment, I created src/sage/namespace/all.py but no src/sage/namespace/__init__.py. I get

sage: import sage.namespace.all
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-1-f30e99bc2b64> in <module>
----> 1 import sage.namespace.all

ModuleNotFoundError: No module named 'sage.namespace.all'

mkoeppe · 2021-11-01T02:42:22Z

comment:24

The current default build system of the Sage library does not actually understand namespace packages. If you look into the installed Sage library in site-packages, you will see that your file all.py has not been copied there. This will be fixed in #28925.

If you want to test this, you can use ./configure --enable-editable to switch to a different build system (https://wiki.sagemath.org/ReleaseTours/sage-9.3#Editable_.28.22in-place.22.2C_.22develop.22.29_installs_of_the_Sage_library)

kwankyu · 2021-11-04T02:38:56Z

comment:25

Replying to @mkoeppe:

The current default build system of the Sage library does not actually understand namespace packages. If you look into the installed Sage library in site-packages, you will see that your file all.py has not been copied there. This will be fixed in #28925.

If you want to test this, you can use ./configure --enable-editable to switch to a different build system (https://wiki.sagemath.org/ReleaseTours/sage-9.3#Editable_.28.22in-place.22.2C_.22develop.22.29_installs_of_the_Sage_library)

Thanks. It works.

tscrim · 2021-11-06T01:36:09Z

comment:44

I don't see a reason why all.py should have its own docstring either. I am +1 for using that.

mkoeppe · 2021-11-06T19:00:35Z

comment:45

Replying to @kwankyu:

Replying to @mkoeppe:

This looks great to me. Ready for review?

We are supposed to deal with all __init__ files. No?

Not necessarily. Only the __init__.py files of packages that will become namespace packages. See explanation in #32501 (also related to our previous discussion in #32734 comment:13).

mkoeppe · 2021-11-06T19:02:50Z

comment:46

Re-running the command from the ticket description of #32501 on the present branch:

$ find src/sage -name '__init__.py' | xargs wc -l | grep -v '^ *0'
       2 src/sage/crypto/__init__.py
       3 src/sage/crypto/mq/__init__.py
       3 src/sage/dynamics/__init__.py
       3 src/sage/dynamics/cellular_automata/__init__.py
       1 src/sage/combinat/words/__init__.py
       1 src/sage/combinat/designs/__init__.py
       1 src/sage/combinat/chas/__init__.py
       1 src/sage/combinat/path_tableaux/__init__.py
       1 src/sage/combinat/species/__init__.py
       1 src/sage/combinat/sf/__init__.py
       1 src/sage/combinat/ncsf_qsym/__init__.py
       6 src/sage/combinat/integer_lists/__init__.py
       9 src/sage/combinat/root_system/__init__.py
       3 src/sage/doctest/__init__.py
     847 src/sage/features/__init__.py
      15 src/sage/repl/__init__.py
       4 src/sage/repl/rich_output/__init__.py
      34 src/sage/__init__.py
     358 src/sage/libs/giac/__init__.py
       2 src/sage/libs/ntl/__init__.py
       1 src/sage/libs/gap/__init__.py
     199 src/sage/libs/pari/__init__.py
      55 src/sage/cpython/__init__.py
       2 src/sage/sat/converters/__init__.py
       4 src/sage/sat/solvers/__init__.py
       1 src/sage/modular/quasimodform/__init__.py
       2 src/sage/finance/__init__.py
       2 src/sage/matroids/__init__.py
     114 src/sage/rings/polynomial/pbori/__init__.py
      10 src/sage/modules/with_basis/__init__.py
       2 src/sage/structure/__init__.py
       2 src/sage/media/__init__.py

mkoeppe · 2021-11-06T19:26:16Z

comment:47

To do:

src/sage/dynamics/__init__.py - because sage.dynamics.complex_dynamics needs sage.symbolic and everything else does not, see Modularization of sagelib: Break out a separate package sagemath-standard-no-symbolics #32601
src/sage/combinat/root_system/__init__.py - because git grep 'cimport' src/sage/combinat/root_system/ reveals from sage.groups.perm_gps.permgroup_element cimport PermutationGroupElement, and git grep 'cimport' src/sage/groups/perm_gps shows dependencies on numerous libraries: gap, flint
src/sage/matroids - to play safe given that there's a lot of cimporting

Not necessary to change:

src/sage/combinat/words/__init__.py because git grep 'cimport' src/sage/combinat/words does not show compile-time dependencies on libraries, so sage.combinat.words does not have to become a namespace package
likewise src/sage/combinat/integer_lists/__init__.py
src/sage/sat/... because git grep 'cimport' src/sage/sat/ is empty
src/sage/features/, src/sage/libs/..., src/sage/repl because they will definitely not become namespace packages

sagetrac-git · 2021-11-08T01:57:35Z

Changed commit from d7085db to b7d2be9

sagetrac-git · 2021-11-08T01:57:35Z

Branch pushed to git repo; I updated commit sha1. New commits:

`b7d2be9`	More refactoring of `__init__` files

kwankyu · 2021-11-08T01:58:04Z

New commits:

`b7d2be9`	More refactoring of `__init__` files

kwankyu · 2021-11-08T01:59:48Z

comment:50

Re-running the command from the ticket description of #32501 on the present branch:

$ find src/sage -name '__init__.py' | xargs wc -l | grep -v '^ *0'
       2 src/sage/crypto/__init__.py
       3 src/sage/crypto/mq/__init__.py
       3 src/sage/dynamics/cellular_automata/__init__.py
      41 src/sage/combinat/words/__init__.py
       1 src/sage/combinat/designs/__init__.py
       1 src/sage/combinat/chas/__init__.py
       1 src/sage/combinat/path_tableaux/__init__.py
       1 src/sage/combinat/species/__init__.py
       1 src/sage/combinat/sf/__init__.py
       1 src/sage/combinat/ncsf_qsym/__init__.py
       6 src/sage/combinat/integer_lists/__init__.py
       9 src/sage/combinat/root_system/__init__.py
       3 src/sage/doctest/__init__.py
     847 src/sage/features/__init__.py
      15 src/sage/repl/__init__.py
       4 src/sage/repl/rich_output/__init__.py
      34 src/sage/__init__.py
     358 src/sage/libs/giac/__init__.py
       2 src/sage/libs/ntl/__init__.py
       1 src/sage/libs/gap/__init__.py
     199 src/sage/libs/pari/__init__.py
      55 src/sage/cpython/__init__.py
       2 src/sage/sat/converters/__init__.py
       4 src/sage/sat/solvers/__init__.py
       1 src/sage/modular/quasimodform/__init__.py
       2 src/sage/finance/__init__.py
       2 src/sage/matroids/__init__.py
     114 src/sage/rings/polynomial/pbori/__init__.py
      10 src/sage/modules/with_basis/__init__.py
       2 src/sage/structure/__init__.py
       2 src/sage/media/__init__.py
    1727 total

mkoeppe · 2021-11-08T02:56:26Z

Reviewer: Matthias Koeppe

mkoeppe · 2021-11-08T03:27:06Z

comment:52

Looking great but I'm getting a docbuild error:

[dochtml] [combinat ] /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages/sage/combinat/crystals/crystals.py:docstring of sage.combinat.crystals.crystals:122: WARNING: undefined label: sage.combinat.crystals
[dochtml] [combinat ] /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages/sage/combinat/quickref.py:docstring of sage.combinat.quickref:48: WARNING: undefined label: sage.combinat.root_system
[dochtml] [combinat ] /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages/sage/combinat/quickref.py:docstring of sage.combinat.quickref:54: WARNING: undefined label: sage.combinat.crystals
[dochtml] [combinat ] /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages/sage/combinat/quickref.py:docstring of sage.combinat.quickref:71: WARNING: undefined label: sage.combinat.posets
[dochtml] [combinat ] /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages/sage/combinat/root_system/root_system.py:docstring of sage.combinat.root_system.root_system:3: WARNING: undefined label: sage.combinat.root_system
[dochtml] [combinat ] dumping search index in English (code: en)... done
[dochtml] [combinat ] The HTML pages are in local/share/doc/sage/html/en/reference/combinat.
[dochtml] Error building the documentation.

sagetrac-git · 2021-11-08T04:18:47Z

Branch pushed to git repo; I updated commit sha1. New commits:

`ee4367d`	`Fixes for doctest failures`

sagetrac-git · 2021-11-08T04:18:47Z

Changed commit from b7d2be9 to ee4367d

kwankyu · 2021-11-08T04:19:43Z

comment:54

Strange that those errors got slipped in unnoticed...

mkoeppe · 2021-11-08T04:45:21Z

comment:55

It's working now, and the built documentation also looks fine. Thanks!

nthiery · 2021-11-08T13:26:35Z

comment:56

Looks good indeed.

Just one thing (that could possibly go in a follow up
ticket): would there be a way to maintain the validity
of cross-references like :ref:sage.combinat ?

First because it's easier to remember, and does not
reveal an implementation detail (that the doc is actually
stored in sage/combinat/all.py).

But also because, thanks to intersphinx links these
cross-references can be used from documents outside
of the Sage documentation, and this is breaking
backward compatibility for them.

Thanks,

kwankyu · 2021-11-09T00:45:37Z

comment:57

Replying to @nthiery:

Looks good indeed.

Just one thing (that could possibly go in a follow up
ticket): would there be a way to maintain the validity
of cross-references like :ref:sage.combinat ?

If you compare "before" and "after" below, we see that Sphinx recognizes regular package and treat it appropriately. Hence I think we need to wait until Sphinx gets equipped with some mechanism to deal with namespace packages appropriately. It seems that Sphinx does not have such a mechanism yet (as far as I know by googling).

Before the patch: this line

.. toctree::    

    sage/combinat/__init__

generates

.. nodoctest

.. _sage.combinat:

Combinatorics
=============

.. This file has been autogenerated.


.. automodule:: sage.combinat.__init__
   :members:
   :undoc-members:
   :show-inheritance:

After the patch: this line

.. toctree::    

    sage/combinat/all

generates

.. nodoctest

.. _sage.combinat.all:

Combinatorics
=============

.. This file has been autogenerated.


.. automodule:: sage.combinat.all
   :members:
   :undoc-members:
   :show-inheritance:

vbraun · 2021-11-12T21:27:46Z

Changed branch from public/32508 to ee4367d

jhpalmieri · 2022-05-29T00:46:14Z

Changed commit from ee4367d to none

jhpalmieri · 2022-05-29T00:46:14Z

comment:59

The addition of

from . import quickref, tutorial

to sage.combinat.all broke the top-level tutorial() command. Maybe that command should be removed anyway, especially since no one has reported its loss, but in any case, right now tutorial() is broken as a command in Sage.

kwankyu · 2022-05-29T06:35:15Z

comment:60

Replying to @jhpalmieri:

The addition of
from . import quickref, tutorial
to sage.combinat.all broke the top-level tutorial() command. Maybe that command should be removed anyway, especially since no one has reported its loss, but in any case, right now tutorial() is broken as a command in Sage.

After 7 months, I don't remember why I put that line into all.py. Simply removing that line would solve the problem. Yes? We don't need quickref either in the global name space.

nthiery · 2022-05-29T10:23:03Z

comment:61

Indeed, there is no reason for quickref and tutorial to be in the global namespace.
All we need is to be able to do sage.combinat? sage.combinat.tutorial? and sage.combinat.quickref?

kwankyu · 2022-05-29T12:05:00Z

comment:62

See #33933.

mkoeppe added this to the sage-9.5 milestone Sep 12, 2021

mkoeppe added t: refactoring labels Sep 12, 2021

kwankyu added s: needs work and removed s: needs info labels Nov 5, 2021

kwankyu added s: needs review and removed s: needs work labels Nov 8, 2021

mkoeppe added s: positive review and removed s: needs review labels Nov 8, 2021

vbraun removed the s: positive review label Nov 12, 2021

vbraun closed this as completed in f4f555c Nov 12, 2021

Eliminate use of __init__.py files for supplying package docstrings #32508

Eliminate use of __init__.py files for supplying package docstrings #32508

Comments

mkoeppe commented Sep 12, 2021

kwankyu commented Oct 28, 2021

kwankyu commented Oct 28, 2021

sagetrac-git mannequin commented Oct 28, 2021

sagetrac-git mannequin commented Oct 28, 2021

kwankyu commented Oct 28, 2021

kwankyu commented Oct 29, 2021

mkoeppe commented Oct 29, 2021

kwankyu commented Oct 29, 2021

kwankyu commented Oct 29, 2021

mkoeppe commented Oct 29, 2021

kwankyu commented Oct 30, 2021

mkoeppe commented Oct 30, 2021

kwankyu commented Oct 30, 2021

kwankyu commented Oct 30, 2021

mkoeppe commented Oct 30, 2021

mkoeppe commented Oct 30, 2021

kwankyu commented Nov 1, 2021

kwankyu commented Nov 1, 2021

mkoeppe commented Nov 1, 2021

kwankyu commented Nov 1, 2021

mkoeppe commented Nov 1, 2021

mkoeppe commented Nov 1, 2021

mkoeppe commented Nov 1, 2021

kwankyu commented Nov 1, 2021

mkoeppe commented Nov 1, 2021

kwankyu commented Nov 4, 2021

tscrim commented Nov 6, 2021

mkoeppe commented Nov 6, 2021

mkoeppe commented Nov 6, 2021

mkoeppe commented Nov 6, 2021

sagetrac-git mannequin commented Nov 8, 2021

sagetrac-git mannequin commented Nov 8, 2021

kwankyu commented Nov 8, 2021

kwankyu commented Nov 8, 2021

mkoeppe commented Nov 8, 2021

mkoeppe commented Nov 8, 2021

sagetrac-git mannequin commented Nov 8, 2021

sagetrac-git mannequin commented Nov 8, 2021

kwankyu commented Nov 8, 2021

mkoeppe commented Nov 8, 2021

nthiery commented Nov 8, 2021

kwankyu commented Nov 9, 2021

vbraun commented Nov 12, 2021

jhpalmieri commented May 29, 2022

jhpalmieri commented May 29, 2022

kwankyu commented May 29, 2022

nthiery commented May 29, 2022

kwankyu commented May 29, 2022

Eliminate use of init.py files for supplying package docstrings #32508

Eliminate use of init.py files for supplying package docstrings #32508