Opened 2 years ago
Last modified 3 months ago
#24067 needs_work enhancement
Call sympy_init() at sympy import
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  interfaces  Keywords:  
Cc:  jdemeyer, isuruf, infinity0, egourgoulhon  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Jeroen Demeyer, Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  u/fbissey/24067 (Commits)  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 (30)
comment:1 Changed 2 years ago by
 Branch set to u/rws/24067
comment:2 Changed 2 years ago by
 Cc jdemeyer added
 Commit set to 40b3432ff5203b94428f482cf0b51360d9edb738
 Dependencies set to #24062
 Description modified (diff)
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Branch changed from u/rws/24067 to u/jdemeyer/24067
comment:4 Changed 2 years ago by
 Commit changed from 40b3432ff5203b94428f482cf0b51360d9edb738 to a97c32f65ea9f194abfb801595ffc6a89c825216
 Dependencies #24062 deleted
 Reviewers set to Jeroen Demeyer
Rebased to 8.1.rc2 for easier reviewing.
New commits:
a97c32f  24067: Call sympy_init() at sympy import

comment:5 Changed 2 years ago by
 Commit changed from a97c32f65ea9f194abfb801595ffc6a89c825216 to 893b03d4d00ea6566648ed315569a46f4096551b
comment:6 Changed 2 years ago by
Positive review to your first commit. I added two followup commits, please review.
comment:7 followup: ↓ 10 Changed 2 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 2 years ago by
What about just adding sympy_init()
to the end of the interface file?
comment:9 in reply to: ↑ 8 Changed 2 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 in reply to: ↑ 7 Changed 2 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Ralf Stephan
 Status changed from needs_review to 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 2 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 2 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 in reply to: ↑ 12 ; followup: ↓ 21 Changed 2 years ago by
 Cc isuruf infinity0 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 2 years ago by
 Branch changed from u/jdemeyer/24067 to u/fbissey/24067
 Commit changed from 893b03d4d00ea6566648ed315569a46f4096551b to 67e6156afccd12253c2c330bff6cb9fdeb11245c
 Status changed from positive_review to needs_work
New commits:
67e6156  Better patch for sympy, that doesn't force sage as a runtime dependency.

comment:15 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:16 Changed 2 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 2 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 2 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 in reply to: ↑ 18 ; followup: ↓ 20 Changed 2 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 in reply to: ↑ 19 Changed 2 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 in reply to: ↑ 13 ; followup: ↓ 24 Changed 2 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 2 years ago by
 Status changed from needs_review to needs_work
If we're going to submit a patch upstream, we shouldn't rush.
comment:23 Changed 2 years ago by
 Description modified (diff)
 Stopgaps set to #24238
comment:24 in reply to: ↑ 21 ; followup: ↓ 29 Changed 2 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 2 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 2 years ago by
comment:27 Changed 2 years ago by
The third option would be a revert ticket for the manifolds changes that introduced it.
comment:28 in reply to: ↑ 26 Changed 2 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 in reply to: ↑ 24 Changed 2 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 months ago by
 Cc egourgoulhon added
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