#11762 closed enhancement (fixed)
More import cleanup
Reported by: | robertwb | Owned by: | jason |
---|---|---|---|
Priority: | major | Milestone: | sage-4.8 |
Component: | misc | Keywords: | |
Cc: | leif, mderickx | Merged in: | sage-4.8.alpha1 |
Authors: | Robert Bradshaw | Reviewers: | Maarten Derickx |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Importing all from __init__.py
simplifies and stabilizes the import order. Unfortunately the current state of imports depends on packages being partially loaded at different points in time, but this patch changes things where it can without breaking sage.
Attachments (1)
Change History (12)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
Cool, could you explain (maybe with a small example if that is not a crazy amount of work) what the precise difference is this makes for importing. I might be ably to figure it out on myself, but since you probably already know it , I don't want to waste my time on thinking to much about the subtle differences between our recursive all.py structure, and just doing import all from init.py?
Or is there really not much of a difference and is this just a first step to better use of init.py?
comment:4 Changed 8 years ago by
Sorry I didn't see your comment earlier.
Basically, when you do import a.b.c
it first loads a/__init__.py
, then a/b/__init__.py
, and finally a/b/c.py
(or a/b/c/__init__.py
).
Importing all
imposes no restraint, so one can import sage.rings.foo.fragile_module
without loading sage.ring.all
. If fragile_module
depends on stable_module
being loaded first, then sage.rings.all
can enforce this order, but only if sage.rings.all
is loaded before import sage.rings.foo.fragile_module
. Even when things aren't fragile, this ensures that the submodules of sage.rings get loaded in the order specified in all.py, greatly reducing the chance that a change in imports in an unrelated portion of code can trigger a chain reaction, import things in a different order, and have result in circular import errors. This will also help with performance, as one can see the actual load time of sage.rings.all without wondering about how many submodules were "preloaded." So it's both a cleanup and a prelude to further cleanup.
comment:5 follow-up: ↓ 11 Changed 8 years ago by
Would something like this also make sense, in some cases:
-
sage/algebras/all.py
diff --git a/sage/algebras/all.py b/sage/algebras/all.py
a b from algebra_element import AlgebraEleme 29 29 from free_algebra import FreeAlgebra, is_FreeAlgebra 30 30 from free_algebra_quotient import FreeAlgebraQuotient 31 31 32 from steenrod .allimport *32 from steenrod import * 33 33 34 34 from group_algebra_new import GroupAlgebra
and then just rename sage/algebras/steenrod/all.py
to sage/algebras/steenrod/__init__.py
?
comment:6 Changed 8 years ago by
Yes, but that would involve changing a lot more code (and people's behavior).
comment:7 Changed 8 years ago by
Thanks for the explanation. It seems that this will indeed make the import order more stable, so from a logical point of view I give it a positive review. Luckily the patch still applies without errors to the just released sage-4.7.2. I'm now running all doctests just to be sure.
comment:8 Changed 8 years ago by
- Status changed from needs_review to positive_review
All test passed on sage.math!
comment:9 Changed 8 years ago by
- Reviewers set to Maarten Derickx
comment:10 Changed 8 years ago by
- Merged in set to sage-4.8.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:11 in reply to: ↑ 5 Changed 8 years ago by
Replying to jhpalmieri:
Would something like this also make sense, in some cases:
sage/algebras/all.py
diff --git a/sage/algebras/all.py b/sage/algebras/all.py
a b from algebra_element import AlgebraEleme 29 29 from free_algebra import FreeAlgebra, is_FreeAlgebra 30 30 from free_algebra_quotient import FreeAlgebraQuotient 31 31 32 from steenrod .allimport *32 from steenrod import * 33 33 34 34 from group_algebra_new import GroupAlgebra and then just rename
sage/algebras/steenrod/all.py
tosage/algebras/steenrod/__init__.py
?
Also, for things like "sage.plot.plot" it could be confusing since if you just did "sage.plot.plot" it would be referring to the function (currently, "sage.plot.all.plot"). However, to import that function directly, it'd be "from sage.plot.plot import plot" which would also be the same as "from sage.plot import plot".
Generated with