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 some monkey patching in src/sage/__init__.py #31420

Closed
mkoeppe opened this issue Feb 20, 2021 · 27 comments
Closed

Remove some monkey patching in src/sage/__init__.py #31420

mkoeppe opened this issue Feb 20, 2021 · 27 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 20, 2021

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

It currently contains:

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Feb 20, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 24, 2021

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.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 24, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Review/remove monkey patching in src/sage/__init__.py Meta-ticket: Review/remove monkey patching in src/sage/__init__.py Sep 5, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@jhpalmieri
Copy link
Member

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

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__.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

@jhpalmieri
Copy link
Member

Commit: ad587cf

@jhpalmieri
Copy link
Member

comment:15

Do you also want del _sys?


New commits:

91ec0f2src/sage/__init__.py: Remove __all__
949d787src/sage/__init__.py: Remove __version__
f77a98esrc/sage/__init__.py: Move 'import zlib' to src/sage/cpython/__init__.py
2e9ea56src/sage/__init__.py: Move monkey-patch of ExtensionFileLoader to src/sage/cpython/__init__.py
ad587cfsrc/sage/__init__.py [Cygwin]: Move monkey-patch of sqlite to src/sage/cpython/__init__.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

Changed commit from ad587cf to af6642f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

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

af6642fsrc/sage/cpython/__init__.py: Clean up sage.cpython module

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Meta-ticket: Review/remove monkey patching in src/sage/__init__.py Remove some monkey patching in src/sage/__init__.py Sep 8, 2021
@kiwifb
Copy link
Member

kiwifb commented Sep 8, 2021

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?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

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.

@kiwifb
Copy link
Member

kiwifb commented Sep 9, 2021

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?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

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

@kiwifb
Copy link
Member

kiwifb commented Sep 9, 2021

comment:23

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

@kiwifb
Copy link
Member

kiwifb commented Sep 9, 2021

Reviewer: François Bissey

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

comment:24

Thanks for reviewing!

@vbraun
Copy link
Member

vbraun commented Sep 15, 2021

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

4 participants