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: sage-8.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 jdemeyer)

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 rws

  • Branch set to u/rws/24067

comment:2 Changed 2 years ago by rws

  • Authors set to Ralf Stephan
  • Cc jdemeyer added
  • Commit set to 40b3432ff5203b94428f482cf0b51360d9edb738
  • Dependencies set to #24062
  • Description modified (diff)
  • Status changed from new to needs_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 2 years ago by jdemeyer

  • Branch changed from u/rws/24067 to u/jdemeyer/24067

comment:4 Changed 2 years ago by jdemeyer

  • Commit changed from 40b3432ff5203b94428f482cf0b51360d9edb738 to a97c32f65ea9f194abfb801595ffc6a89c825216
  • Dependencies #24062 deleted
  • Reviewers set to Jeroen Demeyer

Rebased to 8.1.rc2 for easier reviewing.


New commits:

a97c32f24067: Call sympy_init() at sympy import

comment:5 Changed 2 years ago by git

  • Commit changed from a97c32f65ea9f194abfb801595ffc6a89c825216 to 893b03d4d00ea6566648ed315569a46f4096551b

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 2 years ago by jdemeyer

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

comment:7 follow-up: Changed 2 years ago by 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.

comment:8 follow-up: Changed 2 years ago by tscrim

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

comment:9 in reply to: ↑ 8 Changed 2 years ago by fbissey

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 rws

  • 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 fbissey

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 follow-up: Changed 2 years ago by 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.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by fbissey

  • 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 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 2 years ago by fbissey

  • 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:

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

comment:15 Changed 2 years ago by fbissey

  • Status changed from needs_work to needs_review

comment:16 Changed 2 years ago by infinity0

@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 fbissey

@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 follow-up: Changed 2 years ago by infinity0

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 2 years ago by infinity0 (previous) (diff)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 2 years ago by fbissey

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 2 years ago by rws

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 ; follow-up: Changed 2 years ago by 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.

comment:22 Changed 2 years ago by jdemeyer

  • 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 jdemeyer

  • Description modified (diff)
  • Stopgaps set to #24238

comment:24 in reply to: ↑ 21 ; follow-up: Changed 2 years ago by fbissey

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 2 years ago by tscrim

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 follow-up: Changed 2 years ago by 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.

comment:27 Changed 2 years ago by rws

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 jdemeyer

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 2 years ago by jdemeyer

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 egourgoulhon

  • Cc egourgoulhon added
Note: See TracTickets for help on using tickets.