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

Modify find_python_sources to support modularization of sagelib by native namespace packages (PEP 420) #28925

Closed
mkoeppe opened this issue Dec 30, 2019 · 240 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 30, 2019

This ticket prepares the modularization of sagelib into separate distributions (pip-installable packages) by adding support for native namespace packages (PEP 420) to sagelib's build system.

Ordinary packages (directories of modules) can be turned into namespace packages by removing the empty __init__.py file.
Then several distribution package can share the same namespace.

Because our source discovery mechanism in sagelib's build system (setup.py + sage_setup) distinguishes package directories from other directories containing data files by the presence of the __init__.py file, the present ticket makes several adjustments to the build system - in particular to the functions find_python_sources and find_extra_files.

Per convention in the Sage library, namespace packages are recognized by the presence of all.py or all__*.py files (#33033).

In this ticket, we apply it to turn sagemath-objects and sagemath-categories (from #29865) into namespace packages,
in which __init__.py are removed from the MANIFEST.in files.

Because the actual files are not removed from the source tree, the normal build of the Sage distribution is not affected.

To test these two distributions:

./bootstrap && ./sage -sh -c '(cd pkgs/sagemath-objects && SAGE_NUM_THREADS=16 tox -r -v -v -v -e py39)'

./bootstrap && ./sage -sh -c '(cd pkgs/sagemath-categories && SAGE_NUM_THREADS=16 tox -r -v -v -v -e py39)'

Follow-up tickets:

See also:

Depends on #33803

CC: @isuruf @embray @jdemeyer @dimpase @dcoudert @videlec @vbraun @tobiasdiez @kiwifb

Component: build

Keywords: sd111

Author: Matthias Koeppe

Branch/Commit: f9df1ae

Reviewer: Kwankyu Lee, Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.1 milestone Dec 30, 2019
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 14, 2020

comment:1

pushing these forward to 9.2

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Modify clean_stale_files to support modularization of sagelib by namespace packages Modify clean_stale_files to support modularization of sagelib by native namespace packages (PEP 420) May 17, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Modify clean_stale_files to support modularization of sagelib by native namespace packages (PEP 420) Modify find_python_sources, clean_stale_files to support modularization of sagelib by native namespace packages (PEP 420) May 18, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 20, 2020

Commit: 8bc5662

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 20, 2020

New commits:

8e50bbesage.graphs.graph_decompositions: Make it a namespace package by removing __init__.py (which had unnecessary reimports only)
dff6e62Mark src/sage/ext_data as directories that are not namespace packages
8bc5662sage_setup.find.find_python_sources: Handle native namespace packages and 'nonamespace' files

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 20, 2020

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 20, 2020

Changed commit from 8bc5662 to 606a509

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 20, 2020

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

606a509sage.numerical.backends: Make it a namespace package by removing __init__.py (which was empty)

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 20, 2020

comment:18

Next steps: adjust the cleaner, find Cython sources

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 20, 2020

comment:19

The Cython source finder could perhaps also parse a new directive that controls OptionalExtension behavior (until we get rid of OptionalExtension altogether in #29701) so that we can get rid of module_list.py completely (see #29706)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 20, 2020

Changed commit from 606a509 to a7b2287

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 1, 2022

comment:155

Thanks.

Another comment: clean_install_dir and find_extra_files have a new argument distributions, but the docstrings do not explain it.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2022

Changed commit from cd4d8e9 to 9ab3200

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2022

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

af9ac78src/sage_setup/find.py (find_extra_files): Document new arg 'distributions'
13e66a3src/sage_setup/find.py (find_extra_files): Fix indentation
9ab3200src/sage_setup/clean.py (clean_install_dir): Document new arg 'distributions'

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 4, 2022

Reviewer: Kwankyu Lee, ...

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 4, 2022

comment:157

Thank you for the update.

I have been testing (using) this ticket and and the sequel #33011 for a while. They works well. I checked the code as far as I can (not very far), and it looks good to me.

If we don't have further comments on this ticket, then it would better be tested by more developers in the wild by merging this now, so that we can fix problems before the next release.

I will wait for your (not Matthias) meta comments, and proceed to "positive review" in a couple of days.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 6, 2022

comment:158

Is this

./sage -sh -c '(cd pkgs/sagemath-categories && tox -v -v -v)'  

broken? It doesn't work with (and without) this ticket.

@dimpase
Copy link
Member

dimpase commented Jul 6, 2022

comment:159

Replying to @kwankyu:

Is this

./sage -sh -c '(cd pkgs/sagemath-categories && tox -v -v -v)'  

broken? It doesn't work with (and without) this ticket.

works for me (after make build from a clean state done)

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 6, 2022

comment:160

Replying to @dimpase:

Replying to @kwankyu:

Is this

./sage -sh -c '(cd pkgs/sagemath-categories && tox -v -v -v)'  

broken? It doesn't work with (and without) this ticket.

works for me (after make build from a clean state done)

I did make distclean; make -j8; ./sage -sh -c '(cd pkgs/sagemath-categories && tox -v -v -v)'. So this is clean.

This is the initial part that goes wrong:

sage -t --random-seed=251639358214975434990568781743714015030 --environment=sage.all__sagemath_categories /Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/structure/coerce_actions.pyx
**********************************************************************
File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/structure/coerce_actions.pyx", line 58, in sage.structure.coerce_actions.GenericAction.__init__
Failed example:
    M = MatrixSpace(ZZ,2)
Exception raised:
    Traceback (most recent call last):
      File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.structure.coerce_actions.GenericAction.__init__[0]>", line 1, in <module>
        M = MatrixSpace(ZZ,Integer(2))
    NameError: name 'MatrixSpace' is not defined

Any clue? I am on Mac.

@dimpase
Copy link
Member

dimpase commented Jul 6, 2022

Changed reviewer from Kwankyu Lee, ... to Kwankyu Lee, Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jul 6, 2022

comment:161

I start from git clean -fdx - which cleans much more.

You'll probably want to move upstream/ outside and after git clean make a symbolic link to it, otherwise it will re-download things.

So it's positive review from me.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 6, 2022

comment:162

Replying to @dimpase:

I start from git clean -fdx - which cleans much more.

Thanks. But it didn't help. As far as I understand, the doctest failures are expected. The real problem is:

$ pkgs/sagemath-categories/.tox/py39/bin/python3.9 
Python 3.9.13 (main, May 24 2022, 21:28:31) 
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sage.categories.category import Category
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/py39/lib/python3.9/site-packages/sage/categories/category.py", line 107, in <module>
    from sage.misc.c3_controlled import _cmp_key, _cmp_key_named, C3_sorted_merge
  File "sage/misc/c3_controlled.pyx", line 364, in init sage.misc.c3_controlled (build/cythonized/sage/misc/c3_controlled.c:11819)
  File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/py39/lib/python3.9/site-packages/sage/structure/__init__.py", line 2, in <module>
    import sage.structure.element
  File "sage/structure/element.pyx", line 1, in init sage.structure.element (build/cythonized/sage/structure/element.c:36204)
  File "sage/structure/category_object.pyx", line 60, in init sage.structure.category_object (build/cythonized/sage/structure/category_object.c:9982)
ImportError: cannot import name Category

Strangely this works

Hera:sage-dev$ pkgs/sagemath-categories/.tox/py39/bin/python3.9 
Python 3.9.13 (main, May 24 2022, 21:28:31) 
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sage.structure.sage_object import SageObject
>>> from sage.categories.category import Category
>>> 

Anyway, this is not a problem of this ticket. The situation is the same without this ticket.

Hence my positive review to this ticket!

@dimpase
Copy link
Member

dimpase commented Jul 6, 2022

comment:163

hmm, do you have the current directory, i.e. ., in your PATH? IMHO this might mess things up.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 6, 2022

comment:164

Replying to @dimpase:

hmm, do you have the current directory, i.e. ., in your PATH? IMHO this might mess things up.

Yes. But even after removing it from the path, it's the same.

./configure shows python3-3.10.3: using system package; SPKG will not be installed, and

$ pkgs/sagemath-categories/.tox/py39/bin/python3.9 
Python 3.9.13 (main, May 24 2022, 21:28:31) 
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path
['', '/usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python39.zip', '/usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9', '/usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/lib-dynload', '/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/py39/lib/python3.9/site-packages']
$ pkgs/sagemath-categories/.tox/python/bin/python 
Python 3.10.4 (main, Apr 26 2022, 19:42:59) [Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path
['', '/usr/local/Cellar/python@3.10/3.10.4/Frameworks/Python.framework/Versions/3.10/lib/python310.zip', '/usr/local/Cellar/python@3.10/3.10.4/Frameworks/Python.framework/Versions/3.10/lib/python3.10', '/usr/local/Cellar/python@3.10/3.10.4/Frameworks/Python.framework/Versions/3.10/lib/python3.10/lib-dynload', '/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages']

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2022

comment:165

Replying to @dimpase:

Replying to @kwankyu:

Is this

./sage -sh -c '(cd pkgs/sagemath-categories && tox -v -v -v)'  

broken? It doesn't work with (and without) this ticket.

works for me (after make build from a clean state done)

sagemath-categories (in contrast to sagemath-standard) does not need make build because all of its build and runtime dependencies can be found on PyPI.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2022

comment:166

Replying to @kwankyu:

This is the initial part that goes wrong:

sage -t --random-seed=251639358214975434990568781743714015030 --environment=sage.all__sagemath_categories /Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/structure/coerce_actions.pyx
**********************************************************************
File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/structure/coerce_actions.pyx", line 58, in sage.structure.coerce_actions.GenericAction.__init__
Failed example:
    M = MatrixSpace(ZZ,2)
Exception raised:
    Traceback (most recent call last):
      File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/python/lib/python3.10/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.structure.coerce_actions.GenericAction.__init__[0]>", line 1, in <module>
        M = MatrixSpace(ZZ,Integer(2))
    NameError: name 'MatrixSpace' is not defined

sagemath-categories as of this ticket does not contain src/sage/matrix/matrix_space.py (see pkgs/sagemath-categories/MANIFEST.in), so MatrixSpace is not available in sage.all__sagemath_categories.

Note that the tox run ends with the message "lots of doctest failures are expected". This is one of them.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2022

comment:167

Replying to @kwankyu:

The real problem is:

$ pkgs/sagemath-categories/.tox/py39/bin/python3.9 
Python 3.9.13 (main, May 24 2022, 21:28:31) 
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sage.categories.category import Category
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/py39/lib/python3.9/site-packages/sage/categories/category.py", line 107, in <module>
    from sage.misc.c3_controlled import _cmp_key, _cmp_key_named, C3_sorted_merge
  File "sage/misc/c3_controlled.pyx", line 364, in init sage.misc.c3_controlled (build/cythonized/sage/misc/c3_controlled.c:11819)
  File "/Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-categories/.tox/py39/lib/python3.9/site-packages/sage/structure/__init__.py", line 2, in <module>
    import sage.structure.element
  File "sage/structure/element.pyx", line 1, in init sage.structure.element (build/cythonized/sage/structure/element.c:36204)
  File "sage/structure/category_object.pyx", line 60, in init sage.structure.category_object (build/cythonized/sage/structure/category_object.c:9982)
ImportError: cannot import name Category

Strangely this works

Hera:sage-dev$ pkgs/sagemath-categories/.tox/py39/bin/python3.9 
Python 3.9.13 (main, May 24 2022, 21:28:31) 
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sage.structure.sage_object import SageObject
>>> from sage.categories.category import Category
>>> 

The Sage library contains many cyclic imports. If one starts at the wrong place in an import cycle, one can run into problems with partially initialized modules. See #33580 (Meta-ticket: make all modules importable without sage.all)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 6, 2022

comment:168

Thanks both for the review!

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 7, 2022

comment:169

Replying to @mkoeppe:

Note that the tox run ends with the message "lots of doctest failures are expected".

Yes, it's there. It would perhaps better be colored red or green...

@vbraun
Copy link
Member

vbraun commented Jul 8, 2022

comment:170

Merge failure on top of:

843eb03 Updated SageMath version to 9.7.beta4

merge was not clean: conflicts in pkgs/sagemath-categories/tox.ini

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2022

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

f9df1aeMerge tag '9.7.beta4' into t/28925/modify_find_python_sources__clean_stale_files_to_support_modularization_of_sagelib_by_native_namespace_packages__pep_420_

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2022

Changed commit from 9ab3200 to f9df1ae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants