Remove some monkey patching in src/sage/init.py

Question

Remove some monkey patching in src/sage/init.py

Closed this issue 4 years ago · 27 comments

To make sage a namespace package, we need to get rid of src/sage/__init__.py.

It currently contains:

definition of __all__ (no longer needed; has no effect on completion, neither in plain python3 nor IPython)
definition of __version__ (unused)
preloading of zlib (introduced in #23122)
#32489: load_ipython_extension (introduced in #18726): This function is defined in module sage so that %load_ext sage works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make %unload_ext sage work
#32479: monkey patching of inspect.isfunction to support Cython functions (introduced in #25373)
monkey patching of importlib.machinery.ExtensionFileLoader (introduced in #24681); the quest to get an upstream fix merged was apparently abandoned - python/cpython#6653
monkey patching of sqlite on Cygwin (introduced in #30157)

In this ticket we remove the ones that are not needed any more, and some others to another module init. #32479 and #32489 will take care of the leftovers.

CC: @kiwifb @jhpalmieri @embray

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: af6642f

Reviewer: François Bissey

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

Answer 1 · 2021-02-20T22:14:48.000Z

Description changed:

--- 
+++ 
@@ -3,7 +3,7 @@
 It currently contains:
 - definition of `__all__`
 - definition of `__version__`
-- preloading of zlib
+- preloading of `zlib`
 - `load_ipython_extension`
 - monkey patching of `inspect.isfunction` to support Cython functions
 - monkey patching of sqlite on Cygwin

Answer 2 · 2021-03-24T02:04:25.000Z

comment:2

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

Answer 3 · 2021-09-05T23:54:06.000Z

Description changed:

--- 
+++ 
@@ -5,7 +5,7 @@
 - definition of `__version__`
 - preloading of `zlib`
 - `load_ipython_extension`
-- monkey patching of `inspect.isfunction` to support Cython functions
+- #32479: monkey patching of `inspect.isfunction` to support Cython functions
 - monkey patching of sqlite on Cygwin
 
 We should review all of them, remove anything that is not needed any more, and move the rest to other module inits if possible.

Answer 4 · 2021-09-07T06:03:14.000Z

Description changed:

--- 
+++ 
@@ -3,10 +3,11 @@
 It currently contains:
 - definition of `__all__`
 - definition of `__version__`
-- preloading of `zlib`
-- `load_ipython_extension`
-- #32479: monkey patching of `inspect.isfunction` to support Cython functions
-- monkey patching of sqlite on Cygwin
+- preloading of `zlib` (introduced in #23122)
+- `load_ipython_extension` (introduced in #18726)
+- #32479: monkey patching of `inspect.isfunction` to support Cython functions (introduced in #25373)
+- monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681)
+- monkey patching of sqlite on Cygwin (introduced in #30157)
 
 We should review all of them, remove anything that is not needed any more, and move the rest to other module inits if possible.

Answer 5 · 2021-09-07T06:25:18.000Z

Description changed:

--- 
+++ 
@@ -4,7 +4,7 @@
 - definition of `__all__`
 - definition of `__version__`
 - preloading of `zlib` (introduced in #23122)
-- `load_ipython_extension` (introduced in #18726)
+- `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding method that makes `%unload_ext sage` work
 - #32479: monkey patching of `inspect.isfunction` to support Cython functions (introduced in #25373)
 - monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681)
 - monkey patching of sqlite on Cygwin (introduced in #30157)

Answer 6 · 2021-09-07T06:26:54.000Z

Description changed:

--- 
+++ 
@@ -4,7 +4,7 @@
 - definition of `__all__`
 - definition of `__version__`
 - preloading of `zlib` (introduced in #23122)
-- `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding method that makes `%unload_ext sage` work
+- `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make `%unload_ext sage` work
 - #32479: monkey patching of `inspect.isfunction` to support Cython functions (introduced in #25373)
 - monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681)
 - monkey patching of sqlite on Cygwin (introduced in #30157)

Answer 7 · 2021-09-07T06:55:15.000Z

Description changed:

--- 
+++ 
@@ -6,7 +6,7 @@
 - preloading of `zlib` (introduced in #23122)
 - `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make `%unload_ext sage` work
 - #32479: monkey patching of `inspect.isfunction` to support Cython functions (introduced in #25373)
-- monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681)
+- monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681); the quest to get an upstream fix merged was apparently abandoned - https://github.com/python/cpython/pull/6653
 - monkey patching of sqlite on Cygwin (introduced in #30157)
 
 We should review all of them, remove anything that is not needed any more, and move the rest to other module inits if possible.

Answer 8 · 2021-09-07T06:58:44.000Z

Description changed:

--- 
+++ 
@@ -2,7 +2,7 @@
 
 It currently contains:
 - definition of `__all__`
-- definition of `__version__`
+- definition of `__version__` (unused)
 - preloading of `zlib` (introduced in #23122)
 - `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make `%unload_ext sage` work
 - #32479: monkey patching of `inspect.isfunction` to support Cython functions (introduced in #25373)

Answer 9 · 2021-09-07T07:03:10.000Z

Description changed:

--- 
+++ 
@@ -1,7 +1,7 @@
 To make `sage` a namespace package, we need to get rid of `src/sage/__init__.py`.
 
 It currently contains:
-- definition of `__all__`
+- definition of `__all__` (no longer needed; has no effect on completion, neither in plain python3 nor IPython)
 - definition of `__version__` (unused)
 - preloading of `zlib` (introduced in #23122)
 - `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make `%unload_ext sage` work

Answer 10 · 2021-09-07T23:21:49.000Z

Description changed:

--- 
+++ 
@@ -4,7 +4,7 @@
 - definition of `__all__` (no longer needed; has no effect on completion, neither in plain python3 nor IPython)
 - definition of `__version__` (unused)
 - preloading of `zlib` (introduced in #23122)
-- `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make `%unload_ext sage` work
+- #32489: `load_ipython_extension` (introduced in #18726): This function is defined in module `sage` so that `%load_ext sage` works (see http://www.slabbe.org/blogue/categorie/ipython/); we forgot to define the corresponding function that would make `%unload_ext sage` work
 - #32479: monkey patching of `inspect.isfunction` to support Cython functions (introduced in #25373)
 - monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681); the quest to get an upstream fix merged was apparently abandoned - https://github.com/python/cpython/pull/6653
 - monkey patching of sqlite on Cygwin (introduced in #30157)

Answer 11 · 2021-09-08T20:48:13.000Z

comment:12

It seems that removing __version__ and __all__ is harmless. I don't know about the zlib issue: I tried moving the import to all.py and everything worked, but that's hardly an exhaustive test.

Answer 12 · 2021-09-08T21:17:09.000Z

comment:13

We can probably move it (and also the sqlite monkey-patching on cygwin) to a module that is loaded very early such as sage.cpython.__init__.

Answer 13 · 2021-09-08T21:41:38.000Z

Branch: u/mkoeppe/meta_ticket__review_remove_monkey_patching_in_src_sage___init___py

Answer 14 · 2021-09-08T22:54:42.000Z

Commit: ad587cf

Answer 15 · 2021-09-08T22:54:42.000Z

comment:15

Do you also want del _sys?

New commits:

`91ec0f2`	`src/sage/__init__.py: Remove __all__`
`949d787`	`src/sage/__init__.py: Remove __version__`
`f77a98e`	`src/sage/__init__.py: Move 'import zlib' to src/sage/cpython/__init__.py`
`2e9ea56`	`src/sage/__init__.py: Move monkey-patch of ExtensionFileLoader to src/sage/cpython/__init__.py`
`ad587cf`	`src/sage/__init__.py [Cygwin]: Move monkey-patch of sqlite to src/sage/cpython/__init__.py`

Answer 16 · 2021-09-08T23:03:19.000Z

Changed commit from ad587cf to af6642f

Answer 17 · 2021-09-08T23:03:19.000Z

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

`af6642f`	`src/sage/cpython/__init__.py: Clean up sage.cpython module`

Answer 18 · 2021-09-08T23:23:50.000Z

Author: Matthias Koeppe

Answer 19 · 2021-09-08T23:23:50.000Z

Description changed:

--- 
+++ 
@@ -9,5 +9,6 @@
 - monkey patching of `importlib.machinery.ExtensionFileLoader` (introduced in #24681); the quest to get an upstream fix merged was apparently abandoned - https://github.com/python/cpython/pull/6653
 - monkey patching of sqlite on Cygwin (introduced in #30157)
 
-We should review all of them, remove anything that is not needed any more, and move the rest to other module inits if possible.
+In this ticket we remove the ones that are not needed any more, and some others to another module init. #32479 and #32489 will take care of the leftovers.
 
+

Answer 20 · 2021-09-08T23:33:49.000Z

comment:19

Where does the del _zlib come from in sage/cpython/__init__.py? It wasn't present in sage/__init__.py and the ticket above that group only talks about import zlib. Is mentioning that ticket obsolete? Is the whole block obsolete when we favor system zlib whenever possible?

Answer 21 · 2021-09-09T00:25:21.000Z

comment:20

The del introduced in af6642f just removes the binding of the name _zlib created by the import statement from the sage.cpython module. Likewise the other dels in this commit.

Answer 22 · 2021-09-09T00:32:43.000Z

comment:21

Call me obtuse, why do you import and then delete straight away? Is it a cleaner way to make sure that the right library is loaded while you may not want it around in this particular unit?

Answer 23 · 2021-09-09T00:38:54.000Z

comment:22

The importing there is just to trigger loading some shared library.
One could use a function from importlib instead of using import but that's a tiny bit more obscure

Answer 24 · 2021-09-09T00:41:45.000Z

comment:23

Thank you for taking the time to answer my questions. It looks good to go.

Answer 25 · 2021-09-09T00:41:45.000Z

Reviewer: François Bissey

Answer 26 · 2021-09-09T00:56:29.000Z

comment:24

Thanks for reviewing!

Answer 27 · 2021-09-15T22:06:13.000Z

Changed branch from u/mkoeppe/meta_ticket__review_remove_monkey_patching_in_src_sage___init___py to af6642f