Call sympy_init() at sympy import

[rwst]

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.

CC: @jdemeyer @isuruf @infinity0 @egourgoulhon

Component: interfaces

Stopgaps: #24238

Branch/Commit: u/fbissey/24067 @ 67e6156

Reviewer: François Bissey

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

[rwst]

Branch: u/rws/24067

[rwst]

Commit: 40b3432

[rwst]

Last 10 new commits:

8215d37	Merge branch 'develop' into tmp05
7899ea9	20204: convert SymPy abstract functions
b261ec3	23923: fix doctest
5010acf	Merge branch 'u/rws/23923' of git://trac.sagemath.org/sage into t/20204/20204
14b1c52	20204: more doctests
9c4119a	16801: Conversion of psi(x,y) to/from SymPy
d5c60e3	24062: pkg version/chksum
1cbc23e	24062: remove obsolote patches; adapt remaining
94f359f	24062: doctest fixes
40b3432	24067: Call sympy_init() at sympy import

[rwst]

Dependencies: #24062

[rwst]

Author: Ralf Stephan

[jdemeyer]

Changed branch from u/rws/24067 to u/jdemeyer/24067

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

Changed dependencies from #24062 to none

[jdemeyer]

comment:4

Rebased to 8.1.rc2 for easier reviewing.

---

New commits:

a97c32f

24067: Call sympy_init() at sympy import

[jdemeyer]

Changed commit from 40b3432 to a97c32f

[sagetrac-git]

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

0d4f310	Remove explicit sympy_init() calls
893b03d	Check that sympy is not imported

[sagetrac-git]

Changed commit from a97c32f to 893b03d

[jdemeyer]

comment:6

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

[kiwifb]

comment:7

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.

[tscrim]

comment:8

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

[kiwifb]

comment:9

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.

[rwst]

comment:10

Replying to @kiwifb:

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.

[rwst]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Ralf Stephan

[kiwifb]

comment:11

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

[jdemeyer]

comment:12

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.

[kiwifb]

comment:13

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.

[kiwifb]

Changed commit from 893b03d to 67e6156

[infinity0]

comment:16

@kiwifb, 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.

[kiwifb]

comment:17

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

[infinity0]

comment:18

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?

[kiwifb]

comment:19

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, @rwst do you have an upstream issue for this? There definitely should be.

[rwst]

comment:20

Replying to @kiwifb:

+1, @rwst 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.

[jdemeyer]

comment:21

Replying to @kiwifb:

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.

[jdemeyer]

comment:22

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

[jdemeyer]

Stopgaps: #24238

[kiwifb]

comment:24

Replying to @jdemeyer:

Replying to @kiwifb:

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.

[tscrim]

comment:25

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

[kiwifb]

comment:26

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.

[rwst]

comment:27

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

[jdemeyer]

comment:28

Replying to @kiwifb:

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.

[jdemeyer]

comment:29

Replying to @kiwifb:

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.

[mkoeppe]

Changed author from Ralf Stephan to none

[mkoeppe]

Changed reviewer from Jeroen Demeyer, Ralf Stephan to none

[mkoeppe]

comment:31

Time to close this bizarre ticket

[kiwifb]

comment:32

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

[kiwifb]

Reviewer: François Bissey