Migrate from Maxima pexpect interface to ECL interface #40265
Conversation
|
Documentation preview for this PR (built with commit a4bf6e2; changes) is ready! 🎉 |
|
Tests are green, so this is now ready for review. |
|
is this a total removal, or only making it optional? IIRC maxima pexpect is still used in few places in sagelib, because the glue code needed for library maxima is missing. @nbruin ought to know the details. There is somewhere an issue tracking the work to be done - I recall looking into it, and being unable to set an actionable plan towards completion of this task, as the API is very opaque and undocumented |
There was a problem hiding this comment.
Sorry, but I don't think I'm familiar enough with Maxima or how Sage interfaces to external libraries to review this. This doesn't appear to significantly touch any parts of Sage that I've worked on/with either. Was there any particular part you wanted me to look at?
It's a complete removal of the pexpect interface for maxima. There were a few places that still used it, but they where quite easy to migrate.
I didn't knew which to tag for review and github was suggesting you. Sorry for the noise. |
| sage: f = maxima('x^2 + 17*y^2') | ||
| sage: f = maxima('x^3 + 17*y^2') | ||
| sage: f.diff('x') | ||
| 34*y*'diff(y,x,1)+2*x |
There was a problem hiding this comment.
that's strange. It looks like different interaces have different ideas of what
There was a problem hiding this comment.
This was the only doctest with a really different result. However, I found the previous output very strange as y is considered to be a function of x, but x is not a function of y (see next line in the doctest that takes the derivative wrt to y). So for reasons of symmetry, I find the new output more logical.
There was a problem hiding this comment.
ok, I gather that it must be due to different sets of Maxima packages loaded at startup. So with pexpect interface the evaluation is more lazy, so to speak. No real regression then.
There was a problem hiding this comment.
It's also not the output I find when I try this example just directly in sage. With
maxima.depends('y','x')
maxima('x^2+17*y^2').diff('x')
or
maxima_calculus.depends('y','x')
maxima_calculus('x^2+17*y^2').diff('x')
I'm getting the original result. So in the previous doctest I assume some "depends" statement stuck around from a previous doctest. The only "depends" I can find in the code is implicit in de_solve. So I guess a de_solve doctest ran before?
This is an issue: de_solve messes up the global state. It may well be that de_solve previously still used a non-calculus interface instance (but, as this doctest shows, not a fresh one!), but if this is now the maxima_lib one, the risk of fouling up subsequent results is much larger!
It may be that things are slipping through because de_solve ends up using string conversions to construct variables instead of the "proper" sage transfer mechanism. So de_solve ends up messing up y, whereas a variable y in sage wil normally get translated to _SAGE_VAR_y:
sage: maxima_calculus('y')
y
sage: maxima_calculus(SR('y'))
_SAGE_VAR_y
sage: maxima('y')
y
sage: maxima(SR('y'))
_SAGE_VAR_y
So de_solve's shenanigans may remain unnoticed most of the time because it translates variables in a non-standard way. The maxima manual doesn't use depends when using ode2, so perhaps this routine can be rewritten to avoid the global state problems altogether? Probably for a follow-up ticket.
The change on this ticket changes our potential exposure to this buggy behaviour in de_solve, so some care is appropriate.
|
That's concerning that the functionality produces different results. This might need either a deprecation period, or a fix to have a 1-1 match of results |
|
OK, this is fine. I have some trouble with docs building here, let me double check |
I'm guessing because I've reviewed several code cleanup and typing annotation PRs that touch a lot of different files that GitHub think I'm familiar with a lot more of the codebase than I really am. No worries about the noise. I want to learn more about Sage internals so I might read through whatever discussion happens here once its merged out of curiosity. |
|
I think migrating all maxima use from sage library to maxima_lib makes sense but why eliminate the maxima expect interface? It's a perfectly valid interface that has the advantage that it can interface with any maxima -- not just maxima running on ecl (which is a bit unusual maxima setup) Note, by the way, that maxima_lib can still be used in a string-based fashion. That makes it a drop-in replacement for expect-based. |
|
indeed, we can use non-ecl Maxima with pexpect. Perhaps the correct plan, which does not seem to ask for much extra work here, should be:
(I might be missing something in 3), as there is also |
Right, the pexpect interface would also work with a non-ecl maxima. On the other hand, there is currently no way to install sage without having an ecl-maxima (both sage-the-distro as well as sage-with-meson are compiling ecl+meson if it's not available) - so this advantage is pretty much a theoretical one, I think. Since I couldn't come up with a use case where you would prefer to use the pexpect interface over the library interface, I've removed the former. But I'm not very familiar with this part of sage, don't have a strong opinion and would be happy to restore the old |
I'm actually a bit confused by the |
As SBCL Maxima is often several times as fast as ECL Maxima, if your bottleneck is in Maxima computations (with little data exchange) then you'd get a case to use a non-ECL pexpect Maxima. |
Correct. So I think maintaining |
tobiasdiez
changed the title
Okay, thanks for the input @dimpase and @nbruin! I've now restored the pexpect interface. |
| Clear the maxima variables to avoid interference with other tests:: | ||
| sage: maxima('kill(x,y)') | ||
| done |
There was a problem hiding this comment.
what exactly is the failure? Since none of the code below appears to use something maxima-specific it feels like if the test result depends on some maxima variable it would be a bug (?)
There was a problem hiding this comment.
There are a few other doctests that explicit call maxima using the same variables x,y but require them to be unspecified.
src/sage/interfaces/all.py
Outdated
|
|
||
| from sage.misc.lazy_import import lazy_import | ||
|
|
||
| lazy_import('sage.interfaces.maxima_lib', 'maxima') |
There was a problem hiding this comment.
what happened to the uppercase one?
Also what's the purpose of interfaces/all? Is it exposed to the user? (in which case modification of the behavior would be sort of breaking change, but I doubt user notices since they're both maxima)
Or used internally in some way?
There was a problem hiding this comment.
Also what's the purpose of interfaces/all? Is it exposed to the user?
These are only used to initialize the interactive sage session (what you get if you simply call sage) and the global namespace for the doctests.
|
How about simply getting rid of As well, one can similarly break Sage by redefining Python functions, so I don't really see this possibility to accidentally break things this way as a big problem |
No that has nothing to do with maxima_lib. In fact, that command should just mean: run the "maxima" executable according to the sage environment. Usually that's an ecl-based maxima, but it doesn't need to be; not even for maxima_lib to function as intended, because that one is based on loading some dynamic libraries; not an executable. The main thing I'm saying is "don't advertise So I'd be in favour of repurposing this ticket to: make sure that calculus only uses the maxima_calculus instance that is maxima_lib. Just leave the pexpect interface intact for other uses. |
|
So essentially you are proposing to exhibit the pexecpt interface still as There is indeed the danger that the user changes options or settings in |
This also applies, perhaps to a lesser extend, to the pexpect interface, right? This one also sets some options and preloads some packages, or not? |
I think the settings that are made for the maxima interface are minimal and are mainly related to making the pexpect interface work well (change printing mode; change prompts, etc.) For global state, people can make pexpect interfaces at will and they're not used by the system itself. So users can compartmentalize the global changes as appropriate. For maxima_lib no such option exists. For state in maxima_lib: if people make changes on maxima_calculus, I would expect the changes are meant to impact calculus. If people do it on maxima_lib, it's a bit of a toss-up. If users do it on the global level |
|
I think it's a very messy kind of setup, and I would rather see maxima_lib and maxima_calculus become one, maxima. And maxima becoming maxima_expect. Yes, one can modify settings of maxima(_lib/calculus), just as you can, say, monkey-patch parts of sagelib. Perhaps there should be a function to reset the defaults of maxima(_lib/calculus), to make interactive work easier. If you want a completely separate Maxima session, use maxima_expect. yes, such a change needs deprecation, so let it be. |
|
It sounds like the proposals presented here at the moment are technically the same; it's just the names that are different:
I don't care too much about the names. Personally I don't mind the Presently, there is a (lazily constructed) expect interface exposed in the global namespace: I don't see the benefit of going through renaming the interfaces, since the present names have a good logic behind them. The churn of deprecation doesn't seem to bring a tangible benefit to me. But if someone feels strongly about making the change, they can go ahead as far as I'm concerned. |
|
Okay, I've reverted the global Btw, |
tobiasdiez
force-pushed
the
maxima-bin-remove
branch
from
360583b to
10e716e
Compare
|
Indeed, I don't see the point of (before this PR). Maybe it served some purpose in the past, but it seems to be just a leftover of something. Isn't |
|
It's hard to know if anyone uses it, but I suspect people debugging the maxima use in calculus put a top-level access in for the maxima interface used by calculus for easy access. Yes, it's (by the way, this is one of the few lazy-imports that actually resolves! After access EDIT: never mind. |
| :: | ||
| sage: from sage.interfaces.maxima import maxima |
There was a problem hiding this comment.
I notice that in maxima_lib there is now a doctest that checks that the maxima_lib instance prints as Maxima rather than Maxima_lib. I don't see why that's an improvement so would prefer Maxima_lib to persist, but if you really want to change that, then do add a check here that interfaces.maxima.Maxima instances print in a way distinguishable from how maxima_lib prints.
Given that changing file names and module names would be even more involved, I'd suggest to just stick with the naming of the modules, so then Maxima corresponds to interfaces.maxima and Maxima_lib corresponds to interfaces.maxima_lib. The way these instances print isn't very important for users anyway, so we might as well stick with something that is informative and non-confusing for programmers.
There was a problem hiding this comment.
I've changed that because the interface name was used in a few places to trigger the correct conversation / function calls.
Both are interfaces to "Maxima" so I felt that was not too confusing - if you really need to know which interface you use you could use type(...)
There was a problem hiding this comment.
Can you point out some cases where having them print the same was giving you benefit in how function calls are triggered (or conversation calls, whatever those are)? I have trouble imagining where or how that might be the case and makes me think those spots may benefit from redesign.
There was a problem hiding this comment.
For example in https://github.com/tobiasdiez/sage/blob/ba8b8911d52ce2557e4604e25453b26df7095083/src/sage/calculus/calculus.py#L2669 or https://github.com/tobiasdiez/sage/blob/ba8b8911d52ce2557e4604e25453b26df7095083/src/sage/symbolic/function.pyx#L667. You are probably right that these should be redesigned (i.e. either using the interface as a class, or by directly calling a function on the interface). But I would prefer to not do this here.
There was a problem hiding this comment.
I see: the interface name gets used for finding lookup tables managed by generic interface-related stuff. It does make sense that all maxima interfaces share these tables (apart from the tables that should be instance-specific).
How did this even work with maxima_lib before then? Is that code not triggered with the use of maxima_lib in calculus? I don't recall this coming up when we dropped in maxima_lib for maxima_calculus, but it looks like it should have. So that peaks my personal interest in where you discovered this problem.
EDIT: Ah, I think I see. All uses of _find_var have an explicit string for interface rather than the name of an interface instance. So for that one, the change of the maxima_lib interface name is immaterial.
For the other one, the main use is in sage.symbolic.function.Function._interface_init_ where the interface I gets its name used via: return self._conversions.get(I.name(), self._name). There it does make a difference:
sage: gamma_inc._interface_init_(maxima)
'gamma_incomplete'
sage: gamma_inc._interface_init_(maxima_calculus)
'gamma'
but that is actually due to the name() method -- not its print name:
sage: maxima
Maxima
sage: maxima.name()
'maxima'
sage: maxima_calculus
Maxima_lib
sage: maxima_calculus.name()
'maxima_lib'
So I agree based on this code that changing maxima_lib.name() to "maxima" makes sense. The str of the interface isn't used for that, though, so I don't think you need to change that.
Be prepared that changing this will change the functioning of maxima_lib in calculus and will need extensive testing. There are probably work-arounds in maxima_lib's translation tables (it has some of its own) that work around this long-lived flaw. Whether those would need to be removed ??? The mechanisms there may actually be more efficient anyway ... although populating some of that through the conversion dictionaries on the symbolic functions themselves would be a little more robust.
There was a problem hiding this comment.
Thanks for looking into this so deeply, very much appreciated!
but that is actually due to the name() method -- not its print name:
Sure, but interface is already defining that the printed name is up to capitalization the same as the return value of name:
sage/src/sage/interfaces/interface.py
Lines 85 to 90 in 10e716e
What I could do is to overrite __repr__ in maxima-lib. Is this okay for you?
There was a problem hiding this comment.
Oh boy ... that's a convoluted piece of code. In that case, __name should be harmonized to maxima on maxima_lib because it gets used for function conversion lookups. As a result, the string representation of the interface changes too (and becomes indistinguishable from the one for other maxima interfaces). So I guess that was the change you made. At least now we know why. Do make sure to carefully test if this affects function conversion results for calculus. Surely, this will make function name conversion operate "more correctly" for maxima_lib, but it is a change from how it worked before, so it can break something else.
|
CI is green (modulo the usual random timeouts), so this is ready for review again. |
|
I can reproduce the failure with on my computer WITHOUT this PR, i.e., on 10.8.beta4. Has this been noticed before? |
|
I cannot find a report of it elsewhere, but that doesn't mean much... Thanks for testing that this is not introduced with this PR! |
|
that one is now reported in #40926 , with some further explanation there. |
There was a problem hiding this comment.
Some things I ran into.
I have to say, many trivial changes to the code (like changing the order of imports etc.) slow down the review by quite a bit because one gets swamped in loads of trivial changes.
Most of the line changes are inclusions of imports in doctests which really clutter the diff too, but that's just a consequence of having our docstrings in the code (which does have a lot of other benefits). A tool that would suppress the diffs in docstrings would be useful for reviewing code changes and docstring changes separately. This would be a common python problem. Did someone already bother making such a tool?
| This class should not be instantiated directly, | ||
| but through its subclasses Maxima (Pexpect interface) | ||
| or MaximaLib (library interface):: | ||
| but through its subclass MaximaLib (which is a singleton):: |
There was a problem hiding this comment.
I think this change can be reverted now that the pexpect interface is not disappearing
| sage: f = maxima('x^2 + 17*y^2') | ||
| sage: f = maxima('x^3 + 17*y^2') | ||
| sage: f.diff('x') | ||
| 34*y*'diff(y,x,1)+2*x |
There was a problem hiding this comment.
It's also not the output I find when I try this example just directly in sage. With
maxima.depends('y','x')
maxima('x^2+17*y^2').diff('x')
or
maxima_calculus.depends('y','x')
maxima_calculus('x^2+17*y^2').diff('x')
I'm getting the original result. So in the previous doctest I assume some "depends" statement stuck around from a previous doctest. The only "depends" I can find in the code is implicit in de_solve. So I guess a de_solve doctest ran before?
This is an issue: de_solve messes up the global state. It may well be that de_solve previously still used a non-calculus interface instance (but, as this doctest shows, not a fresh one!), but if this is now the maxima_lib one, the risk of fouling up subsequent results is much larger!
It may be that things are slipping through because de_solve ends up using string conversions to construct variables instead of the "proper" sage transfer mechanism. So de_solve ends up messing up y, whereas a variable y in sage wil normally get translated to _SAGE_VAR_y:
sage: maxima_calculus('y')
y
sage: maxima_calculus(SR('y'))
_SAGE_VAR_y
sage: maxima('y')
y
sage: maxima(SR('y'))
_SAGE_VAR_y
So de_solve's shenanigans may remain unnoticed most of the time because it translates variables in a non-standard way. The maxima manual doesn't use depends when using ode2, so perhaps this routine can be rewritten to avoid the global state problems altogether? Probably for a follow-up ticket.
The change on this ticket changes our potential exposure to this buggy behaviour in de_solve, so some care is appropriate.
| 0.0 | ||
| """ | ||
| return self.maxima_methods().rectform() | ||
| return self._maxima_().rectform()._sage_() |
There was a problem hiding this comment.
I see maxima_wrapper was removed. What was the reason for doing so?
6 participants
Fixes #17753 and fixes #30071 by migrating the last usages of the pexpect interface to
maxima_lib.📝 Checklist
⌛ Dependencies