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

Remove __init__.py files for subpackages designated to be namespace packages #33011

Closed
mkoeppe opened this issue Dec 11, 2021 · 103 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 11, 2021

This ticket changes the following packages to namespace packages (by removing __init__.py):

A complication lies in the behavior of the Python import machinery:
setup.py puts the source path in front of setup.py because sage_setup uses sage.env and is_package_or_sage_namespace_package_dir (#33033).
But when an old version of sage that is an ordinary package is installed already, the source will not shadow it.

To avoid this complication, in this ticket we do not yet remove src/sage/__init__.py. That is done in #34187.

CC: @tobiasdiez @kwankyu

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 39aa2f1

Reviewer: Kwankyu Lee

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

@mkoeppe mkoeppe added this to the sage-9.6 milestone Dec 11, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 11, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 11, 2021

Last 10 new commits:

2a4a323Merge tag '9.5.beta7' into t/29039/pip_installable_package_sage_bootstrap
9ef0567Merge #29039
e0399a1pkgs/sagemath-objects: Install a fixed set of scripts, not all of src/bin
7519048src/bin/sage[-env]: Put SAGE_ROOT/src/bin in front of path only if run out of this directory
00024e6Merge #32933
bfc953bsrc/sage/doctest/reporting.py: Indicate --environment if not default
36d6b09pkgs/sagemath-categories/setup.cfg.m4: Add scripts
8648741Merge #29865
d91e23bMerge #28925
137f5eeRemove __init__.py files

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 11, 2021

Commit: 137f5ee

@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 Dec 17, 2021

Changed commit from 137f5ee to 699ca47

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2021

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

2e995ddsrc/sage/{arith,ext,libs,rings,sets}/all__sagemath_objects.py
f244b57pkgs/sagemath-objects/MANIFEST.in: Exclude generated files
2f6d473pkgs/sagemath-categories/MANIFEST.in.m4: Exclude generated files
1a3afe6Merge #28925
7f9c9b0Remove __init__.py files
cf7371fsrc/sage_setup/find.py: Update import of is_package_or_namespace_package_dir
5808f3dsrc/sage/misc/package_dir.py: New file for is_package_or_sage_namespace_package_dir
f5addb3Merge #33033
464b0a5Merge #28925
699ca47Remove __init__.py files, add some all.py files

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2021

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

2062384src/sage/__init__.py: Restore

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 17, 2021

Changed commit from 699ca47 to 2062384

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 17, 2021

Author: Matthias Koeppe

@mkoeppe mkoeppe changed the title Remove __init__.py files for packages designated to be namespace packages Remove __init__.py files for subpackages designated to be namespace packages Dec 17, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2022

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

3939af7Merge tag '9.5.rc3' into t/33033/prepare_sage_doctest_for_namespace_packages
178d111Merge #33033
efbc045Merge #28925

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2022

Changed commit from 2062384 to efbc045

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2022

Changed commit from efbc045 to be39670

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2022

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

5c873e0Merge tag '9.6.beta1' into t/28925/modify_find_python_sources__clean_stale_files_to_support_modularization_of_sagelib_by_native_namespace_packages__pep_420_
37b3299pkgs/sagemath-{objects,categories}/setup.cfg.m4: Update python_requires
5546c63pkgs/sagemath-objects/MANIFEST.in: Add sage.misc.package_dir
be39670Merge #28925

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2022

Work Issues: broken with "configure --enable-editable" (gitpod)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2022

Changed commit from be39670 to 87277b3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2022

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

f961a7dsrc/setup.py: Support namespace packages in Cython 0.x
87277b3Merge #28925

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2022

Changed work issues from broken with "configure --enable-editable" (gitpod) to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2022

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

81237e1Merge tag '9.7.beta2' into t/28925/modify_find_python_sources__clean_stale_files_to_support_modularization_of_sagelib_by_native_namespace_packages__pep_420_
dd2c525Merge tag '9.7.beta3' into t/28925/modify_find_python_sources__clean_stale_files_to_support_modularization_of_sagelib_by_native_namespace_packages__pep_420_
2cc9002pkgs/sagemath-objects/setup.py: Remove unused import
cd4d8e9pkgs/sagemath-standard/setup.py: Monkey-patching cython is now done by the sage_build_cython command
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'
6a76779Merge #28925

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2022

Changed commit from 56e9123 to 6a76779

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 4, 2022

comment:65

Replying to @kwankyu:

Replying to @mkoeppe:

Replying to @kwankyu:

When I rebuild sage by sage -b, I observe much bigger log, including

...
[sagelib-9.7.beta1] package init file 'sage/misc/__init__.py' not found (or not a regular file)
[sagelib-9.7.beta1] package init file 'sage/numerical/__init__.py' not found (or not a regular file)
[...]

These messages are normal output when namespace packages are in use.

They are not very informative and look like error messages.

setuptools 63.1.0 will eliminate them - https://setuptools.pypa.io/en/latest/history.html#v63-1-0

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 4, 2022

comment:66

Replying to @mkoeppe:

Replying to @kwankyu:

Replying to @mkoeppe:

Replying to @kwankyu:

When I rebuild sage by sage -b, I observe much bigger log, including

...
[sagelib-9.7.beta1] package init file 'sage/misc/__init__.py' not found (or not a regular file)
[sagelib-9.7.beta1] package init file 'sage/numerical/__init__.py' not found (or not a regular file)
[...]

These messages are normal output when namespace packages are in use.

They are not very informative and look like error messages.

setuptools 63.1.0 will eliminate them - https://setuptools.pypa.io/en/latest/history.html#v63-1-0

Nice!

Those changing mode of ... lines are not seen any more already.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2022

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

bd185b5Merge tag '9.7.beta5' into t/33011/remove___init___py_files_for_packages_designated_to_be_namespace_packages

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2022

Changed commit from 6a76779 to bd185b5

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 11, 2022

Changed dependencies from #28925 to none

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:69

I get

sage -t --warn-long 67.8 --random-seed=272636909256794490188818216866457263419 src/sage_setup/find.py
**********************************************************************
File "src/sage_setup/find.py", line 406, in sage_setup.find.installed_files_by_module
Failed example:
    files_by_module['sage.graphs.graph_decompositions']
Expected:
    set()
Got:
    {'sage/graphs/graph_decompositions/__init__.py',
     'sage/graphs/graph_decompositions/__pycache__/__init__.cpython-39.pyc'}
**********************************************************************
1 item had failures:
   1 of  10 in sage_setup.find.installed_files_by_module
    [49 tests, 1 failure, 1.77 s]
----------------------------------------------------------------------
sage -t --warn-long 67.8 --random-seed=272636909256794490188818216866457263419 src/sage_setup/find.py  # 1 doctest failed
----------------------------------------------------------------------

There is no __init__.py in src/sage/graphs/graph_decompositions, but there is one in local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/graphs/graph_decompositions. Even if I delete it, it gets created when I rebuild sage by sage -b.

Do I need to make distclean first?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 12, 2022

comment:70

Hm, I guess the __init__.py file survives in pkgs/sagemath-standard/build/lib.*

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 12, 2022

comment:71

make distclean (or just rm -rf pkgs/sagemath-standard/build) will solve it. Not sure if there's a good way to make it work with incremental builds

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:72

Replying to @mkoeppe:

Hm, I guess the __init__.py file survives in pkgs/sagemath-standard/build/lib.*

Yes. it's there.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:73

Replying to @mkoeppe:

make distclean (or just rm -rf pkgs/sagemath-standard/build) will solve it. Not sure if there's a good way to make it work with incremental builds

I deleted the directory pkgs/sagemath-standard/build and did make -j8. This will save time, but I don't know if this is a valid way to build sage.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:74

Replying to @kwankyu:

Replying to @mkoeppe:
(or just rm -rf pkgs/sagemath-standard/build)

Ah, you mentioned this.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 13, 2022

comment:75

Replying to @kwankyu:

I get

sage -t --warn-long 67.8 --random-seed=272636909256794490188818216866457263419 src/sage_setup/find.py
**********************************************************************
File "src/sage_setup/find.py", line 406, in sage_setup.find.installed_files_by_module
Failed example:
    files_by_module['sage.graphs.graph_decompositions']
Expected:
    set()
Got:
    {'sage/graphs/graph_decompositions/__init__.py',
     'sage/graphs/graph_decompositions/__pycache__/__init__.cpython-39.pyc'}

How about adding a comment like

     files_by_module['sage.graphs.graph_decompositions']  # make distclean if failed

to guide people what to do to resolve the failure?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2022

Changed commit from bd185b5 to 39aa2f1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2022

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

39aa2f1src/sage_setup/command/sage_install.py (sage_clean): Clean __init__.py when installed package was ordinary, source package is namespace

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 13, 2022

comment:77

I think I found a proper solution for this

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 15, 2022

comment:78

Replying to @mkoeppe:

I think I found a proper solution for this

Yes. It looks good.

I have some questions. The file sage/__init__.py is not deleted yet here. Then can a subpackage of sage be a namespace package? I guess the answer is yes, but no two distribution packages can install into the namespace package. Am I right?

Is the complication of removing sage/__init__.py still valid?

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 15, 2022

comment:79

Replying to @kwankyu:

Is the complication of removing sage/__init__.py still valid?

I am asking this even though I do not understand the complication (I tried to).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 15, 2022

comment:80

Replying to @kwankyu:

The file sage/__init__.py is not deleted yet here. Then can a subpackage of sage be a namespace package? I guess the answer is yes, but no two distribution packages can install into the namespace package. Am I right?

That's right.

Is the complication of removing sage/__init__.py still valid?

Yes, I've opened #34187 for it

@mkoeppe

This comment has been minimized.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 15, 2022

comment:81

I think sage is now ready to get this in.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 15, 2022

Reviewer: Kwankyu Lee

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 15, 2022

comment:82

Thank you!

@vbraun
Copy link
Member

vbraun commented Jul 21, 2022

@vbraun vbraun closed this as completed in ea11a51 Jul 21, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
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

3 participants