Opened 5 years ago

Closed 13 months ago

#24067 closed enhancement (invalid)

Call sympy_init() at sympy import

Reported by: Ralf Stephan Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: interfaces Keywords:
Cc: Jeroen Demeyer, Isuru Fernando, Ximin Luo, Eric Gourgoulhon Merged in:
Authors: Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: u/fbissey/24067 (Commits, GitHub, GitLab) Commit: 67e6156afccd12253c2c330bff6cb9fdeb11245c
Dependencies: Stopgaps: #24238

Status badges

Description (last modified by Jeroen Demeyer)

sympy_init() enables conversion of SymPy objects. It is called with the first initialization of a SympyConverter, i.e. with Expression._sympy_(). Better is to only call it once with any import of SymPy.

We should also revert the patch added in #24238.

Change History (33)

comment:1 Changed 5 years ago by Ralf Stephan

Branch: u/rws/24067

comment:2 Changed 5 years ago by Ralf Stephan

Authors: Ralf Stephan
Cc: Jeroen Demeyer added
Commit: 40b3432ff5203b94428f482cf0b51360d9edb738
Dependencies: #24062
Description: modified (diff)
Status: newneeds_review

Last 10 new commits:

8215d37Merge branch 'develop' into tmp05
7899ea920204: convert SymPy abstract functions
b261ec323923: fix doctest
5010acfMerge branch 'u/rws/23923' of git://trac.sagemath.org/sage into t/20204/20204
14b1c5220204: more doctests
9c4119a16801: Conversion of psi(x,y) to/from SymPy
d5c60e324062: pkg version/chksum
1cbc23e24062: remove obsolote patches; adapt remaining
94f359f24062: doctest fixes
40b343224067: Call sympy_init() at sympy import

comment:3 Changed 5 years ago by Jeroen Demeyer

Branch: u/rws/24067u/jdemeyer/24067

comment:4 Changed 5 years ago by Jeroen Demeyer

Commit: 40b3432ff5203b94428f482cf0b51360d9edb738a97c32f65ea9f194abfb801595ffc6a89c825216
Dependencies: #24062
Reviewers: Jeroen Demeyer

Rebased to 8.1.rc2 for easier reviewing.


New commits:

a97c32f24067: Call sympy_init() at sympy import

comment:5 Changed 5 years ago by git

Commit: a97c32f65ea9f194abfb801595ffc6a89c825216893b03d4d00ea6566648ed315569a46f4096551b

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

0d4f310Remove explicit sympy_init() calls
893b03dCheck that sympy is not imported

comment:6 Changed 5 years ago by Jeroen Demeyer

Positive review to your first commit. I added two follow-up commits, please review.

comment:7 Changed 5 years ago by François Bissey

I am concerned that the sympy patch cannot be sent upstream as is. It makes sage a dependency of sympy for it to even start. I doubt that's acceptable.

comment:8 Changed 5 years ago by Travis Scrimshaw

What about just adding sympy_init() to the end of the interface file?

comment:9 in reply to:  8 Changed 5 years ago by François Bissey

Replying to tscrim:

What about just adding sympy_init() to the end of the interface file?

I would favor an all in sage solution to this. I am not against a push in sympy upstream but it has to be acceptable to them and that will certainly not be.

comment:10 in reply to:  7 Changed 5 years ago by Ralf Stephan

Reviewers: Jeroen DemeyerJeroen Demeyer, Ralf Stephan
Status: needs_reviewpositive_review

Replying to fbissey:

I am concerned that the sympy patch cannot be sent upstream as is. It makes sage a dependency of sympy for it to even start. I doubt that's acceptable.

To make it work the __init__ file needs to know if it is imported from sage. I'm no Python expert but I doubt that's possible. A solution to the design problem would be not to have _sage_ member functions but a global sage(...) method.

Additions LGTM.

comment:11 Changed 5 years ago by François Bissey

What about guarding it with a try: expect: block so it can start gracefully if sage is not present (and therefore not used).

comment:12 Changed 5 years ago by Jeroen Demeyer

fbissey: I am assuming that this ticket would only be merged in some 8.2.beta of Sage. Since this branch here does solve the immediate problem of docbuilding, I suggest to merge this patch as-is and in the mean time work on a solution for upstream.

comment:13 in reply to:  12 ; Changed 5 years ago by François Bissey

Cc: Isuru Fernando Ximin Luo added

Replying to jdemeyer:

fbissey: I am assuming that this ticket would only be merged in some 8.2.beta of Sage. Since this branch here does solve the immediate problem of docbuilding, I suggest to merge this patch as-is and in the mean time work on a solution for upstream.

I was hoping a more conciliatory attitude and hadn't removed the positive review. This is now back to need_work. This is just un-acceptable to sage-on-distro this close to release.

We would have to make our own version of the patch that does the "right thing" (TM) instead of a "sloppy job" (TM) just because we can and feel in too much of a hurry for 3 more lines of code, anyway so let's do it right now instead.

comment:14 Changed 5 years ago by François Bissey

Branch: u/jdemeyer/24067u/fbissey/24067
Commit: 893b03d4d00ea6566648ed315569a46f4096551b67e6156afccd12253c2c330bff6cb9fdeb11245c
Status: positive_reviewneeds_work

New commits:

67e6156Better patch for sympy, that doesn't force sage as a runtime dependency.

comment:15 Changed 5 years ago by François Bissey

Status: needs_workneeds_review

comment:16 Changed 5 years ago by Ximin Luo

@fbissey, that last commit, while much better than what was before, will still force all users of sympy to import sage into their python program, as long as sage is installed.

A better method might be something like this instead:

if 'sage.XXXX' in sys.modules:
    from sage.interfaces.sympy import sympy_init
    sympy_init()

where XXXX is some module that sage always imports. So the patch is only activated if sage is already imported.

This might be more acceptable to upstream as well.

comment:17 Changed 5 years ago by François Bissey

@infinity0 I agree that is not perfect - for exactly the reason you mention - but that's the best I could come up with without more complication. Feel free to improve it, that's one of the reason I called other packagers on this ticket.

comment:18 Changed 5 years ago by Ximin Luo

So what about using if ... in sys.modules?

If you additionally need to support the use-case where a third-party program/library really has to import sympy then later import sage (I can't imagine why), then the if ... in sys.modules logic needs to also be added to a sage module but in reverse - i.e. check if sympy is already imported, and activate the patch if so.

In any case, we're unlikely to patch sympy in Debian like this, unless they indicate that they would already accept it. (Someone else maintains it, the situation is outside of my control.) Could you/someone else file a upstream ticket *before* releasing this in Sage?

Last edited 5 years ago by Ximin Luo (previous) (diff)

comment:19 in reply to:  18 ; Changed 5 years ago by François Bissey

Replying to infinity0:

So what about using if ... in sys.modules?

I can make that change, but I am calling it a night :)

If you additionally need to support the use-case where a third-party program/library really has to import sympy then later import sage (I can't imagine why), then the if ... in sys.modules logic needs to also be added to a sage module but in reverse - i.e. check if sympy is already imported, and activate the patch if so.

In any case, we're unlikely to patch sympy in Debian like this, unless they indicate that they would already accept it. (Someone else maintains it, the situation is outside of my control.) Could you/someone else file a upstream ticket *before* releasing this in Sage?

+1, @rws do you have an upstream issue for this? There definitely should be.

comment:20 in reply to:  19 Changed 5 years ago by Ralf Stephan

Replying to fbissey:

+1, @rws do you have an upstream issue for this? There definitely should be.

I'll open one if this ticket has code that all agree on. Set at least needs review so we have the patchbots.

comment:21 in reply to:  13 ; Changed 5 years ago by Jeroen Demeyer

Replying to fbissey:

This is just un-acceptable to sage-on-distro

Even for a beta version of Sage? I agree that it would be unacceptable in a released version.

comment:22 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

If we're going to submit a patch upstream, we shouldn't rush.

comment:23 Changed 5 years ago by Jeroen Demeyer

Description: modified (diff)
Stopgaps: #24238

comment:24 in reply to:  21 ; Changed 5 years ago by François Bissey

Replying to jdemeyer:

Replying to fbissey:

This is just un-acceptable to sage-on-distro

Even for a beta version of Sage? I agree that it would be unacceptable in a released version.

Come on. We are at rc2. We are just ironing bugs, that would end up in the released 8.1 unless you moved the milestone to 8.2.

comment:25 Changed 5 years ago by Travis Scrimshaw

I would be surprised if this was positively reviewed it would make it into 8.1 (unless we set it as a blocker).

comment:26 Changed 5 years ago by François Bissey

Because of the troubles building the documentation reported on sage-release, one of #24238 or this ticket has to be a blocker in my opinion. I came to this ticket after Jeroen commented on #24238 that this ticket should be the one with the fix.

comment:27 Changed 5 years ago by Ralf Stephan

The third option would be a revert ticket for the manifolds changes that introduced it.

comment:28 in reply to:  26 Changed 5 years ago by Jeroen Demeyer

Replying to fbissey:

Because of the troubles building the documentation reported on sage-release, one of #24238 or this ticket has to be a blocker in my opinion. I came to this ticket after Jeroen commented on #24238 that this ticket should be the one with the fix.

#24238 is a blocker. That ticket is also simple enough that it shouldn't cause problems.

comment:29 in reply to:  24 Changed 5 years ago by Jeroen Demeyer

Replying to fbissey:

We are just ironing bugs, that would end up in the released 8.1 unless you moved the milestone to 8.2.

No. I'm pretty sure that the release manager only merges "blocker" tickets once an rc has been released. The whole point of an "rc" is that it should be close to a release and you don't want to merge random tickets at that point.

Anyway, that doesn't matter anymore for this ticket.

comment:30 Changed 3 years ago by Eric Gourgoulhon

Cc: Eric Gourgoulhon added

comment:31 Changed 15 months ago by Matthias Köppe

Authors: Ralf Stephan
Milestone: sage-8.1sage-duplicate/invalid/wontfix
Reviewers: Jeroen Demeyer, Ralf Stephan
Status: needs_workneeds_review

Time to close this bizarre ticket

comment:32 Changed 15 months ago by François Bissey

Reviewers: François Bissey
Status: needs_reviewpositive_review

I am not remembering much about it but obviously it didn't make the cut then.

comment:33 Changed 13 months ago by Matthias Köppe

Resolution: invalid
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.