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

Explicitly set MPMATH_SAGE env variable #32799

Closed
antonio-rojas opened this issue Oct 30, 2021 · 18 comments
Closed

Explicitly set MPMATH_SAGE env variable #32799

antonio-rojas opened this issue Oct 30, 2021 · 18 comments

Comments

@antonio-rojas
Copy link
Contributor

Since mpmath 1.2.1, specifically since commit [1], mpmath checks for the SAGE_ROOT or MPMATH_SAGE env variables in order to enable sage types instead of relying on whether sage.all can be imported.

This breaks distro packages where SAGE_ROOT is undefined. I reported this upstream a while ago [2] with not much luck. Here we explicitly define MPMATH_SAGE in sage-env so mpmath uses Sage types in a Sage session even if SAGE_ROOT is not defined.

This is still broken when using sage as a python library (in which case sage-env is not read at all), but fixing that use case is beyond the scope of this ticket.

[1] mpmath/mpmath@84ca137
[2] mpmath/mpmath#574

CC: @mkoeppe @kiwifb @dimpase @isuruf

Component: packages: standard

Author: Antonio Rojas

Branch/Commit: d4b1b60

Reviewer: François Bissey

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

@antonio-rojas antonio-rojas added this to the sage-9.5 milestone Oct 30, 2021
@antonio-rojas
Copy link
Contributor Author

@antonio-rojas
Copy link
Contributor Author

Author: Antonio Rojas

@antonio-rojas
Copy link
Contributor Author

New commits:

c2cde67Explicitly set MPMATH_SAGE env variable

@antonio-rojas

This comment has been minimized.

@antonio-rojas
Copy link
Contributor Author

Commit: c2cde67

@slel
Copy link
Member

slel commented Oct 30, 2021

comment:3

See also

@kiwifb
Copy link
Member

kiwifb commented Oct 30, 2021

comment:4

This is the minimum to do vanilla sage. I have already endorsed the approach in principle months ago. I am not sure I would check MPMATH_SAGE first since I'd always want it to be 1, unless I explicitly want to break things.

I would like more opinion on that one because not providing a way to override is contentious.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 30, 2021

comment:5

We should probably set this environment variable in the Sage Python library, not in the sage-env script.

@kiwifb
Copy link
Member

kiwifb commented Oct 30, 2021

comment:6

Replying to @mkoeppe:

We should probably set this environment variable in the Sage Python library, not in the sage-env script.

I agree with that. If I knew how to do it, there would already be a ticket for it ready to review. The stuff I tried didn't work, the only things I can think of require cooperation from mpmath devs.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 30, 2021

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

d4b1b60Set the env variable in the Sage python library

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 30, 2021

Changed commit from c2cde67 to d4b1b60

@antonio-rojas
Copy link
Contributor Author

comment:8

Replying to @kiwifb:

I agree with that. If I knew how to do it, there would already be a ticket for it ready to review. The stuff I tried didn't work, the only things I can think of require cooperation from mpmath devs.

Does this not work for you?

@kiwifb
Copy link
Member

kiwifb commented Oct 30, 2021

comment:9

No, because it is too late to set an environment variable. You are already in the application and it is run with the variables it was launched with. You would need a python variable to tell mpmath that it has been imported from sage.

@kiwifb
Copy link
Member

kiwifb commented Oct 30, 2021

comment:10

It would work if you started a new application like what happens in sage/interface/*.py with pexpect but not while running.

@antonio-rojas
Copy link
Contributor Author

comment:11

Replying to @kiwifb:

No, because it is too late to set an environment variable. You are already in the application and it is run with the variables it was launched with. You would need a python variable to tell mpmath that it has been imported from sage.

Can you give more details on how exactly it is failing for you? Because everything seems to work fine for me (both doctests and running stuff from a sage session)

@kiwifb
Copy link
Member

kiwifb commented Oct 30, 2021

Reviewer: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Oct 30, 2021

comment:12

OK, it surprisingly works. I must have had a messed up environment when I tried it.

@vbraun
Copy link
Member

vbraun commented Nov 2, 2021

Changed branch from u/arojas/explicitly_set_mpmath_sage_env_variable to d4b1b60

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

5 participants