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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix slow Sage imports #76

Closed
wants to merge 5 commits into from
Closed

Fix slow Sage imports #76

wants to merge 5 commits into from

Conversation

grhkm21
Copy link
Contributor

@grhkm21 grhkm21 commented Jul 30, 2023

By replacing all Sage imports with an individual imoprt, the loading time for from estimator import * reduces from 3.5s to 1.4s. Doctests seem to work (except for timeout stuffs) so I assume I didn't break anything 馃槃

grhkm21 and others added 2 commits July 30, 2023 20:39
By replacing all Sage imports with an individual imoprt, the loading
time for `from estimator import *` reduces from 3.5s to 1.4s.
Fix duplicate import
@bencrts
Copy link
Collaborator

bencrts commented Aug 22, 2023

I guess we would need the CI to pass before we used an alternative technique for importing (see here: https://github.com/malb/lattice-estimator/pull/76/checks). It seems to be unhappy with the way infinity is imported (I can't tell why at first look).

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 22, 2023

The PEP8 fails because of F401 'sage.rings.all.QQ' imported but unused, but this is necessary due to Sage shenanigans. I just overwrote it with # noqa, which allows the check to be performed for other import statements.

Regarding the infinity import problem, I have no idea why. It works for me locally:

sage -python -c "from sage.rings.infinity import infinity as oo" 

@malb
Copy link
Owner

malb commented Aug 22, 2023

Perhaps the CI uses a different Sage version? Those deep imports are not supported by Sage, ie it gives no guarantee those will work.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 22, 2023

The most recent log prints this at the very start:


Run export SAGE_ROOT=`sage -c "import os; print(os.environ['SAGE_ROOT'])" | tail -1`
Traceback (most recent call last):
  File "/usr/bin/sage-eval", line 10, in <module>
    eval(compile(s,'<cmdline>','exec'))
  File "<cmdline>", line 1, in <module>
  File "/usr/lib/python3.10/os.py", line 680, in __getitem__
    raise KeyError(key) from None
KeyError: 'SAGE_ROOT'

which seems weird. Also, CI seems to specify that Sage is installed via apt-get install, which from my past experience severely lags behind. It says on this website that it can be anywhere from Sage 9.0 to 10.0...
Is it possible to make CI use the sage docker image instead, or would that be too much work? This PR isn't that important after all

@malb
Copy link
Owner

malb commented Aug 23, 2023

Indeed, this is installing sagemath (9.5-4). I'm hesitant to make changes that will break on Sage versions this recent, just imagine all the panicking support requests we'd have to deal with.

@grhkm21
Copy link
Contributor Author

grhkm21 commented Aug 23, 2023

This is very weird, I tried using a fresh Sage 9.0 (conda) installation and there's no issue with importing. Not sure then...

I also noticed that at the start of the log, it prints an error where SAGE_ROOT env variable is not set, not sure if that is related.

I will close this issue but if I figure out why this is happening I will report back

@grhkm21 grhkm21 closed this Aug 23, 2023
grhkm21 pushed a commit to grhkm21/lattice-estimator that referenced this pull request Aug 23, 2023
@malb
Copy link
Owner

malb commented Aug 24, 2023

FWIW, removing the SAGE_ROOT stuff from the tests doesn't affect them: #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants