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

Replace imports from sage.all #123

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

mkoeppe
Copy link

@mkoeppe mkoeppe commented Oct 29, 2024

Preparation for using modularized distributions of the Sage library.

Done in part using sage -fiximports.

@unhyperbolic
Copy link
Member

unhyperbolic commented Oct 31, 2024

Thanks for looking into this.

How stable are these import paths? How many versions of sage do they go back?

The CI is configured to run SnapPy against various docker containers with different SageMath versions. It reports that 4 out of 11 suites failed. Unfortunately, github doesn't let me see which :(

In the past, we had large try: ... except ImportError: ... blocks to account for different sage versions.

@mkoeppe
Copy link
Author

mkoeppe commented Oct 31, 2024

How stable are these import paths? How many versions of sage do they go back?

AFAICS these imports are all unchanged in Sage over at least the past 5 years

The CI is configured to run SnapPy against various docker containers with different SageMath versions. It reports that 4 out of 11 suites failed. Unfortunately, github doesn't let me see which :(

Are you saying that the current branch was already tested?

By the way, my motivation here is to get SnapPy to run on top of my modularized fork of Sage https://github.com/passagemath/passagemath

@NathanDunfield
Copy link
Member

NathanDunfield commented Oct 31, 2024

Our CI tests SnapPy on the official Sage Docker images, from 9.3 (circa 2021) up to 10.4. We do not test against the development branch of Sage.

I enabled the CI for this PR and tweaked the CI config. The PR fails when running the doctests for Sage 9.* and passes for Sage 10.*. The common error is:

  File "/home/sage/sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/snappy/snap/polished_reps.py", line 18, in <module>
    from sage.combinat.subset import powerset
ImportError: cannot import name 'powerset' from 'sage.combinat.subset' (/home/sage/sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/combinat/subset.py)

@mkoeppe
Copy link
Author

mkoeppe commented Oct 31, 2024

Thanks @NathanDunfield. Would changes like just pushed in ada49ea be acceptable?

@NathanDunfield
Copy link
Member

Would changes like just pushed in ada49ea be acceptable?

Overall, it looks good to me. For the frequently imported things, it might be better to do the import in sage_helper.py , especially if it needs a try-except block. See the example of ComplexField in that file.

@mkoeppe
Copy link
Author

mkoeppe commented Nov 1, 2024

Here's a more complete version (still has a failure in doctesting snappy.database that I don't understand at the moment).
With this version, I can get snappy.test to run on top of only passagemath-flint, passagemath-repl, passagemath-groups, passagemath-symbolics, passagemath-plot, passagemath-graphs.

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.

3 participants