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

circular import in maxima_lib.py #16520

Closed
rwst opened this issue Jun 23, 2014 · 7 comments
Closed

circular import in maxima_lib.py #16520

rwst opened this issue Jun 23, 2014 · 7 comments

Comments

@rwst
Copy link

rwst commented Jun 23, 2014

The lazy import in #2516 uncovered bad imports in interfaces/maxima_lib.py. To enable fixing it independently of a review of that ticket a separate ticket is created.

Component: interfaces

Keywords: maxima, cleanup

Author: Ralf Stephan

Branch/Commit: u/rws/circular_import_in_maxima_lib_py @ a3bc86c

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

@rwst rwst added this to the sage-6.3 milestone Jun 23, 2014
@rwst
Copy link
Author

rwst commented Jun 23, 2014

@rwst
Copy link
Author

rwst commented Jun 23, 2014

Commit: a3bc86c

@rwst
Copy link
Author

rwst commented Jun 23, 2014

New commits:

a3bc86c16520: fix whole file imports

@rwst
Copy link
Author

rwst commented Jun 23, 2014

Author: Ralf Stephan

@nbruin
Copy link
Contributor

nbruin commented Jun 23, 2014

comment:3

Just out of curiosity, do you have a theory why using from ... import ... works better than import ...? I would normally expect that from ... import ... constructions are more fragile (although it probably doesn't matter if the imported symbols are accessed in the module initialization code anyway)

@nbruin
Copy link
Contributor

nbruin commented Jun 23, 2014

comment:4

I noticed these executed at runtime (as opposed to module initialization time):

+ from sage.rings.rational import Rational
+ from sage.rings.number_field.number_field_element_quadratic import NumberFieldElement_quadratic
+ if isinstance(obj, Rational):

That's bad, because these are global assignments. You don't want to do those
again and again. Also, I find it hard to believe that those two modules somehow
refer back to maxima_lib.

The following runs at module initialization time anyway. I don't see how from ... makes this better than import sage.symbolic.pynac. It's not critical, so
I don't really care, apart from the fact that the namespace gets an extra
unnecessary symbol symbol_table.

+from sage.symbolic.pynac import symbol_table
+max_to_pynac_table = symbol_table['maxima']

In general, I have trouble seeing how maxima_lib can lead to circular imports,
because maxima_lib is an interface and as such gets referenced by virtually
no-one. The only place is sage.calculus.calculus and I've very carefully made
sure we don't reference that. Furthermore, even there it's imported lazily, and
for very good reason: Importing maxima_lib initializes ecl with maxima
which takes considerable time.

So I suspect it's not so much a circular import but a bad interaction between
the lazy import of maxima_lib and the attempted lazy import of
hypergeometric. The proper solution is probably to not lazily import
hypergeometric into maxima_lib, but import it plainly there. By the time you're
importing maxima_lib you're already committed to significant initialization
time, so unless hypergeometric does something really strange, I don't expect it
will have a significant effect.

TL;DR: I'm not convinced that this ticket is actually solving a real problem.

addition: I think the following gives a strong indication that maxima_lib does not have a circular import. The following is supposed to be a complete list of the imports done in maxima_lib. A circular import would imply that one of those imports leads to importing maxima_lib, which means the LazyImport should resolve:

sage: from sage.symbolic.ring import SR, var
sage: from sage.interfaces.maxima_abstract import (MaximaAbstract, MaximaAbstractFunction,
....:   MaximaAbstractElement, MaximaAbstractFunctionElement,
....:   MaximaAbstractElementFunction)
sage: import sage.rings.real_double
sage: import sage.symbolic.expression
sage: import sage.functions.trig
sage: import sage.functions.log
sage: import sage.functions.other
sage: import sage.symbolic.integration.integral
sage: from sage.symbolic.operators import FDerivativeOperator
sage: from sage.symbolic.ring import is_SymbolicVariable
sage: type(sage.calculus.calculus.maxima)
<type 'sage.misc.lazy_import.LazyImport'>

This could just be a stale LazyImport proxy, but it's not. Note the runtime needed to compute the hash for the first time. This is indicating the ecl initialization is only run then.

sage: %time hash(sage.calculus.calculus.maxima)
CPU times: user 0.77 s, sys: 0.02 s, total: 0.79 s
Wall time: 0.80 s
-7971541566211231133

The symbol is now resolved and indeed, hash is instantaneous:

sage: type(sage.calculus.calculus.maxima)
<class 'sage.interfaces.maxima_lib.MaximaLib'>
sage: %time hash(sage.calculus.calculus.maxima)
CPU times: user 0.00 s, sys: 0.00 s, total: 0.00 s
Wall time: 0.00 s
-7971541566211231133

There are stale lazyimport proxies lying around:

sage: type(maxima_calculus)
<type 'sage.misc.lazy_import.LazyImport'>
sage: hash(maxima_calculus)
-7971541566211231133
sage: type(maxima_calculus)
<type 'sage.misc.lazy_import.LazyImport'>

That's now #16522

@rwst
Copy link
Author

rwst commented Jun 24, 2014

comment:5

Thanks for your thorough review. I assume this one can be closed as invalid then.

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