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:  sageduplicate/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 
Description (last modified by )
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
Branch:  → u/rws/24067 

comment:2 Changed 5 years ago by
Authors:  → Ralf Stephan 

Cc:  Jeroen Demeyer added 
Commit:  → 40b3432ff5203b94428f482cf0b51360d9edb738 
Dependencies:  → #24062 
Description:  modified (diff) 
Status:  new → needs_review 
comment:3 Changed 5 years ago by
Branch:  u/rws/24067 → u/jdemeyer/24067 

comment:4 Changed 5 years ago by
Commit:  40b3432ff5203b94428f482cf0b51360d9edb738 → a97c32f65ea9f194abfb801595ffc6a89c825216 

Dependencies:  #24062 
Reviewers:  → Jeroen Demeyer 
Rebased to 8.1.rc2 for easier reviewing.
New commits:
a97c32f  24067: Call sympy_init() at sympy import

comment:5 Changed 5 years ago by
Commit:  a97c32f65ea9f194abfb801595ffc6a89c825216 → 893b03d4d00ea6566648ed315569a46f4096551b 

comment:6 Changed 5 years ago by
Positive review to your first commit. I added two followup commits, please review.
comment:7 followup: 10 Changed 5 years ago by
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 followup: 9 Changed 5 years ago by
What about just adding sympy_init()
to the end of the interface file?
comment:9 Changed 5 years ago by
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 Changed 5 years ago by
Reviewers:  Jeroen Demeyer → Jeroen Demeyer, Ralf Stephan 

Status:  needs_review → positive_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
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 followup: 13 Changed 5 years ago by
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 asis and in the mean time work on a solution for upstream.
comment:13 followup: 21 Changed 5 years ago by
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 asis 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 unacceptable to sageondistro 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
Branch:  u/jdemeyer/24067 → u/fbissey/24067 

Commit:  893b03d4d00ea6566648ed315569a46f4096551b → 67e6156afccd12253c2c330bff6cb9fdeb11245c 
Status:  positive_review → needs_work 
New commits:
67e6156  Better patch for sympy, that doesn't force sage as a runtime dependency.

comment:15 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:16 Changed 5 years ago by
@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
@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 followup: 19 Changed 5 years ago by
So what about using if ... in sys.modules
?
If you additionally need to support the usecase where a thirdparty 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?
comment:19 followup: 20 Changed 5 years ago by
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 usecase where a thirdparty 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 Changed 5 years ago by
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 followup: 24 Changed 5 years ago by
Replying to fbissey:
This is just unacceptable to sageondistro
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
Status:  needs_review → needs_work 

If we're going to submit a patch upstream, we shouldn't rush.
comment:23 Changed 5 years ago by
Description:  modified (diff) 

Stopgaps:  → #24238 
comment:24 followup: 29 Changed 5 years ago by
Replying to jdemeyer:
Replying to fbissey:
This is just unacceptable to sageondistro
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
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 followup: 28 Changed 5 years ago by
comment:27 Changed 5 years ago by
The third option would be a revert ticket for the manifolds changes that introduced it.
comment:28 Changed 5 years ago by
Replying to fbissey:
Because of the troubles building the documentation reported on sagerelease, 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 Changed 5 years ago by
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
Cc:  Eric Gourgoulhon added 

comment:31 Changed 15 months ago by
Authors:  Ralf Stephan 

Milestone:  sage8.1 → sageduplicate/invalid/wontfix 
Reviewers:  Jeroen Demeyer, Ralf Stephan 
Status:  needs_work → needs_review 
Time to close this bizarre ticket
comment:32 Changed 15 months ago by
Reviewers:  → François Bissey 

Status:  needs_review → positive_review 
I am not remembering much about it but obviously it didn't make the cut then.
comment:33 Changed 13 months ago by
Resolution:  → invalid 

Status:  positive_review → closed 
Last 10 new commits:
Merge branch 'develop' into tmp05
20204: convert SymPy abstract functions
23923: fix doctest
Merge branch 'u/rws/23923' of git://trac.sagemath.org/sage into t/20204/20204
20204: more doctests
16801: Conversion of psi(x,y) to/from SymPy
24062: pkg version/chksum
24062: remove obsolote patches; adapt remaining
24062: doctest fixes
24067: Call sympy_init() at sympy import