Opened 18 months ago

Closed 12 months ago

Last modified 11 months ago

#31553 closed enhancement (fixed)

Maxima SPKG: eliminate undoing_true_false_printing_patch.patch

Reported by: Michael Orlitzky Owned by:
Priority: major Milestone: sage-9.5
Component: distribution Keywords:
Cc: François Bissey, Dima Pasechnik, Matthias Köppe, Antonio Rojas, Erik Bray, Karl-Dieter Crisman, Isuru Fernando, Marius Gerbershagen Merged in:
Authors: Michael Orlitzky Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: a842fc0 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

Our maxima SPKG requires(?) a custom patch called undoing_true_false_printing_patch.patch that reverts this upstream commit:

https://sourceforge.net/p/maxima/code/ci/f5e9b0f7eb122c4e48ea9df144dd57221e5ea0ca

Our patch was introduced in sage commit 91990e962b to ease the upgrade to maxima-5.29.1. Since upstream has no plans to revert that commit (and distros won't either), we'll have to work around it before we can use maxima from the system.

Change History (30)

comment:1 Changed 18 months ago by Michael Orlitzky

Maybe this isn't so bad. If I delete the patch, I get one doctest failure:

sage -t --long --random-seed=0 src/sage/calculus/calculus.py
**********************************************************************
File "src/sage/calculus/calculus.py", line 1208, in sage.calculus.calculus.limit
Failed example:
    limit(x^a, x=0)
Expected:
    Traceback (most recent call last):
    ...
    ValueError: Computation failed since Maxima requested additional
    constraints; using the 'assume' command before evaluation *may* help
    (example of legal syntax is 'assume(a>0)', see `assume?` for
     more details)
    Is a an even number?
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/mjo/src/sage.git/local/lib/python3.8/site-packages/sage/interfaces/maxima_lib.py", line 984, in sr_limit
        return max_to_sr(maxima_eval(([max_limit], L)))
      File "sage/libs/ecl.pyx", line 854, in sage.libs.ecl.EclObject.__call__ (build/cythonized/sage/libs/ecl.c:8628)
        return ecl_wrap(ecl_safe_apply(self.obj,(<EclObject>lispargs).obj))
      File "sage/libs/ecl.pyx", line 365, in sage.libs.ecl.ecl_safe_apply (build/cythonized/sage/libs/ecl.c:5910)
        raise RuntimeError("ECL says: {}".format(
    RuntimeError: ECL says: Maxima asks: Is _SAGE_VAR_a an even\ number?
    <BLANKLINE>
    During handling of the above exception, another exception occurred:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/mjo/src/sage.git/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/mjo/src/sage.git/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.calculus.calculus.limit[15]>", line 1, in <module>
        limit(x**a, x=Integer(0))
      File "/home/mjo/src/sage.git/local/lib/python3.8/site-packages/sage/calculus/calculus.py", line 1394, in limit
        l = maxima.sr_limit(ex, v, a)
      File "/home/mjo/src/sage.git/local/lib/python3.8/site-packages/sage/interfaces/maxima_lib.py", line 988, in sr_limit
        self._missing_assumption(s)
      File "/home/mjo/src/sage.git/local/lib/python3.8/site-packages/sage/interfaces/maxima_lib.py", line 1033, in _missing_assumption
        raise ValueError(outstr)
    ValueError: Computation failed since Maxima requested additional constraints; using the 'assume' command before evaluation *may* help (example of legal syntax is 'assume(a>0)', see `assume?` for more details)
    Is a an even\ number?
**********************************************************************

One backslash to get rid of. What could possibly go wrong?

comment:2 Changed 18 months ago by Matthias Köppe

Milestone: sage-9.3sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

comment:3 Changed 18 months ago by Michael Orlitzky

I'm not getting too far with this, but the problem lies somewhere between when the retrieve function receives its msg argument in interfaces/maxima_lib.py and when it is printed to the console. The msg we receive is

((MTEXT) Is  $_SAGE_VAR_a  an  EVEN   NUMBER ?)

but when printed out it looks like

Is _SAGE_VAR_a an even\ number?

Our copy of retrieve is out-of-date, but the upstream one has the same issue. I tried setting the global value of linel to 1000 in case it was a word-wrap issue in one of the display functions, but no luck there.

comment:4 Changed 18 months ago by Michael Orlitzky

I should mention: the two lines above are the difference between (princ msg) and (displa msg).

comment:5 Changed 13 months ago by Matthias Köppe

Milestone: sage-9.4sage-9.5

comment:6 Changed 13 months ago by Michael Orlitzky

I may as well admit defeat here. I dumped many hours into this before, and a few more today, and I'm still no closer to understanding what's going wrong. The following workaround moves the patch into sagelib, so that we can accept the system maxima:

  • src/sage/interfaces/maxima_lib.py

    diff --git a/src/sage/interfaces/maxima_lib.py b/src/sage/interfaces/maxima_lib.py
    index 14be2195f5..4dfc3b69e8 100644
    a b ecl_eval('(defun principal nil (cond ($noprincipal (diverg)) ((not pcprntd) (mer 
    113113ecl_eval("(remprop 'mfactorial 'grind)") # don't use ! for factorials (#11539)
    114114ecl_eval("(setf $errormsg nil)")
    115115
     116# This next definition overrides maxima's msize-mtext to revert
     117#
     118#   https://sourceforge.net/p/maxima/code/ci/f5e9b0f7eb122c4e48
     119#
     120# All we (intend to) do is replace two "stringp" with "atom". How this
     121# prevents newlines/backslashes from leaking into the output, I cannot
     122# say; but monkey-patching it here avoids the need to patch maxima
     123# itself. See trac 31553 for discussion. And for the love of god,
     124# please someone fix this before it comes back to haunt us.
     125ecl_eval(r"""
     126(defun msize-mtext (x l r)
     127  (setq x (cdr x))
     128  (if (null x)
     129      (msz nil l r)
     130      (do ((nl) (w 0))
     131          ((null (cdr x))
     132           (setq nl (cons (if (atom (car x))
     133                              (msz (makestring (car x)) l r)
     134                              (msize (car x) l r lop rop))
     135                          nl))
     136           (cons (+ w (caar nl)) (nreverse nl)))
     137        (setq nl (cons (if (atom (car x))
     138                           (msz (makestring (car x)) l r)
     139                           (msize (car x) l r lop rop))
     140                       nl)
     141              w (+ w (caar nl))
     142              x (cdr x)
     143              l nil))))
     144""")
     145
    116146# the following is a direct adaptation of the definition of "retrieve"
    117147# in the Maxima file macsys.lisp. This routine is normally responsible
    118148# for displaying a question and returning the answer. We change it to

If no one smarter than me can figure it out, I'll eventually post a branch with that change so that we can get started working on a system-maxima branch.

comment:7 Changed 13 months ago by Matthias Köppe

Cc: Marius Gerbershagen added

comment:8 Changed 12 months ago by Michael Orlitzky

Authors: Michael Orlitzky
Branch: u/mjo/ticket/31553
Commit: bfb921df49da1ac26535e82f3721417da804ca02
Status: newneeds_review

New commits:

bfb921dTrac #31553: move custom patch into maxima_lib initialization.

comment:9 Changed 12 months ago by Nils Bruin

If it helps anything, I suspect the backslash insertion may be coming from this patch. Note that grind.lisp has its own slash, but it looks like both should backslash a space. Looking at the MTEXT you get, the double space suggests the object is ( ... "EVEN " "NUMBER" ...) or ( ... "EVEN" " NUMBER" ...). Also, these may be symbols that have attributes on them that tell grind/displa` how to render them to strings (internationalization in maxima already??).

The line where the mtext gets constructed is here.

Quite frankly, is maxima normally is happy with the backslashed space in the string it constructs (because their printer strips out the backslash then again, presumably) we should be happy with it too. The only thing we're doing in maxima_lib is monkey-patch retrieve to raise an error with the question string rather than ask the question. If we need to do a little bit of work on that string to make it look nice (like unescape spaces) then we should. So I think what should happen is:

  • confirm that unpatched maxima normally asks the question without a backspace in it.
  • update our monkey-patched retrieve to include the string in its error message as produced by unpatched maxima. Ideally we'd do that by using the same rendering to a string that maxima does to the IO stream, but it may be that the different streams aren't fully compatible. In that case, we may want to do some post processing on the string. This is certainly a cleaner solution than (monkey-)patching an additional internal routine.

I think you may want to reconsider the final goal of using an unpatched system maxima, though: we build maxima.fas through a custom extension of the maxima build system. Maxima normally does not build it, because its build process is set up to produce an *application* not a *library*. Our build instructions are ECL-specific (and indeed, it's not immediately clear that all lisps would support something like this), so I don't think that you'll find much traction pushing this change upstream, much less getting distributions to build Maxima/ECL this way. You'd basically have to make a separate package maxima-ecl-fas.rpm or something like that.

I expect we're stuck with a custom-built maxima for that reason already: maxima_lib is NOT a "supported" way of using maxima.

So it's nice to clean up patches like the one on this ticket for general code maintainability, but I think it's futile to try and move to a "standard" maxima. (I hope that a sage install that uses the system ECL without write permission on the system ECL library can somehow adapt ECLs search paths to also look in a location that sits in the sage tree)

comment:10 in reply to:  9 ; Changed 12 months ago by Michael Orlitzky

Replying to nbruin:

If it helps anything, ... So I think what should happen is...

That sounds reasonable to me, but it's more work than I was prepared to bite off. While monkey-patching is terrible, it's no worse than patch-patching, so I don't think this makes the situation worse than it was.

I think you may want to reconsider the final goal of using an unpatched system maxima, though: we build maxima.fas through a custom extension of the maxima build system.

Yeah, I know. But at least Gentoo, Conda, and Nix all ship the same patch, and maxima upstream might be willing to add it as a new ./configure option if we ask nicely. The spkg-configure.m4 for maxima would check that the system copy was usable in any case, so while we may not be able to use the system copy everywhere, being able to use it on Gentoo, Conda, Nix, etc. would be an improvement.

If maxima upstream strongly rejects the idea, then in the long term, I think we should revert back to a supported interface (but expect lots of disagreement on that) rather than require a custom patched maxima on most systems for the rest of eternity.

I hope that a sage install that uses the system ECL without write permission on the system ECL library can somehow adapt ECLs search paths to also look in a location that sits in the sage tree

We already support system ECL and this works because you can point ECL to the maxima library directly if you've just installed the maxima SPKG and know where it lives.

comment:11 in reply to:  10 ; Changed 12 months ago by Nils Bruin

Replying to mjo:

If maxima upstream strongly rejects the idea, then in the long term, I think we should revert back to a supported interface (but expect lots of disagreement on that) rather than require a custom patched maxima on most systems for the rest of eternity.

Upstream ticket for this: https://sourceforge.net/p/maxima/patches/80/

I think reverting maxima to a "supported" interface is a no-go. Then we should move to sympy or something else as default back-end for "hard calculus". The only "supported" way of communicating with maxima would be through an expect interface. These are fragile and also need constant care; I expect much more than this little element: this is the first time this one plays up in the roughly 10 years we've had maxima_lib. That's a pretty good score. Remember that maxima is a very old, very mature project. The fundamentals are *very* unlikely to change, because there is probably no-one anymore who fully knows how the fundamentals are put together. The reality is that probably any other project would require *more* work to stay up-to-date with. If that project would provide more/better/more stable functionality that would be worth it.

It definitely needs to be *possible* for sage to have specific components: we'll need them in the future as projects go defunct or undergo drastic redesigns that make the new version unusable for our purposes. It's nice if we can use system-provided resources if they are compatible, but there is a reason why sage originally bundled everything: it's very hard to coordinate compatible versions of prerequisites if you don't have the capability of building them yourself if need be. Don't be lured into a false sense of security by seeing some prerequisite projects that are enjoying a period of relative stability. That WILL pass and then we need that prereq built custom, at least to bridge the period where it's unstable and/or sage is adapting to the "new" way of doing it.

comment:12 Changed 12 months ago by Nils Bruin

OK, I think I found what the problem really is. If I capture what msg is actually passed to retrieve (that is to say: I monkey-patch retrieve to store the message in a global variable), I get:

sage: ecl_eval("*RET_MSG*") 
<ECL: ((MTEXT) "Is " |$_SAGE_VAR_a| " an " EVEN | | NUMBER "?")>

The vertical bars note the enclosed string is actually the name of a symbol, not a string. So it passes the space as a symbol. And apparently when this gets processed by maxima and sent to a string-backed stream rather than STDOUT, this gets rendered as "\ " rather than " ". There may be fancier options, but as long as this is the only problem we have, a plain work-around is to manually replace space symbols by strings before processing. Updated "retrieve":

ecl_eval(r"""
(defun retrieve (msg flag &aux (print? nil))
  (declare (special msg flag print?))
  (setq msg (mapcar #'(lambda (x) (if (eq x '| |) " " x)) msg))
  (or (eq flag 'noprint) (setq print? t))
  (error
      (concatenate 'string "Maxima asks: "
      (string-trim '(#\Newline)
      (with-output-to-string (*standard-output*)
      (cond ((not print?)
         (setq print? t)
             (format-prompt t ""))
        ((null msg)
             (format-prompt t ""))
        ((atom msg)
             (format-prompt t "~A" msg)
         (mterpri))
        ((eq flag t)
             (format-prompt t "~{~A~}" (cdr msg))
         (mterpri))
        (t
             (format-prompt t "~M" msg)
         (mterpri))
      ))))
  )
)
""")

This should work with an unpatched msize-mtext.

*FOR POSTERITY*

To help people hack ecl/maxima from sage/python in the future: The key to capturing the message was to do a

ecl_eval(r"""(defvar *RET_MSG* NIL)""")

and then stuff the following somewhere at the start of retrieve:

(setq *RET_MSG* msg)

This can all be done interactively from the sage prompt by a judicious use of ecl_eval statements, because LISP is just about as dynamic as PYTHON is. For posterity, the command sequence to get the offending error message:

var('x,a')
assume(a>0)
assume(a,'integer')
limit(x^(a),x=0)

comment:13 in reply to:  12 Changed 12 months ago by Karl-Dieter Crisman

*FOR POSTERITY*

Thank you for this, from at least one of our future selves!

comment:14 Changed 12 months ago by git

Commit: bfb921df49da1ac26535e82f3721417da804ca02d71fcbb7bed39e74dfa68cdfededb1890c5b54b5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d71fcbbTrac #31553: work around space symbols in maxima_lib's "retrieve" function.

comment:15 in reply to:  12 Changed 12 months ago by Michael Orlitzky

Replying to nbruin:

There may be fancier options, but as long as this is the only problem we have, a plain work-around is to manually replace space symbols by strings before processing. Updated "retrieve":

Brilliant, I've updated the branch and expect good things from ptestlong.

comment:16 Changed 12 months ago by git

Commit: d71fcbb7bed39e74dfa68cdfededb1890c5b54b56e214ae79a4788ff2911699fd35b579a86165b95

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6e214aeTrac #31553: work around space symbols in maxima_lib's "retrieve" function.

comment:17 in reply to:  11 Changed 12 months ago by Michael Orlitzky

Replying to nbruin:

Upstream ticket for this: https://sourceforge.net/p/maxima/patches/80/

Robert Dodier just responded to my comment on that ticket. If anyone has any additional questions or reservations about that patch, now would be the time to speak up.

comment:18 Changed 12 months ago by Nils Bruin

Maxima has just merged a ticket https://sourceforge.net/p/maxima/patches/102/ changing the | | to a " ". That should obviate the need for the "mapcar" line once we upgrade to the next releast (>5.45.1)

This is now #32611.

comment:19 Changed 12 months ago by Michael Orlitzky

Patchbot is green if anyone is willing to rubber-stamp this. And Nils, please add yourself as an author if you wish.

comment:20 Changed 12 months ago by Matthias Köppe

Branch: u/mjo/ticket/31553u/mkoeppe/ticket/31553

comment:21 Changed 12 months ago by Matthias Köppe

Commit: 6e214ae79a4788ff2911699fd35b579a86165b957a4ed0d6dddaeab25d51d74096d288297e2be6e1

The indentation of the Lisp code was unusual, probably from a mixture of tabs & spaces somewhere in the process. Fixed.


New commits:

7a4ed0dsrc/sage/interfaces/maxima_lib.py: Re-indent lisp code

comment:22 Changed 12 months ago by git

Commit: 7a4ed0d6dddaeab25d51d74096d288297e2be6e1a842fc0d159d9a183f60a6795836a7693ee11a65

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

a842fc0build/pkgs/maxima: Bump patch level

comment:23 Changed 12 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

comment:24 in reply to:  21 Changed 12 months ago by Michael Orlitzky

Replying to mkoeppe:

The indentation of the Lisp code was unusual, probably from a mixture of tabs & spaces somewhere in the process. Fixed.

Whoops, thanks!

comment:25 in reply to:  21 Changed 12 months ago by Nils Bruin

Replying to mkoeppe:

The indentation of the Lisp code was unusual, probably from a mixture of tabs & spaces somewhere in the process. Fixed.


New commits:

7a4ed0dsrc/sage/interfaces/maxima_lib.py: Re-indent lisp code

No not really. The indentation was to closely match that of the place where the code is copy-pasted from. The nature of lisp code makes it so that indenting 4 spaces for every parenthesis level quickly runs you into the right margin, so some creativity (or 2 space indents) is required.

comment:26 Changed 12 months ago by Matthias Köppe

The original https://github.com/andrejv/maxima/blob/master/src/macsys.lisp#L330 uses canonical indentation [assuming a tab width of 8]; and so does my version.

comment:27 Changed 12 months ago by François Bissey

It's good to get rid of that particular patch. What is happening with the "other" patch, namely matrixexp.patch? Is there a ticket for it that escaped my attention?

comment:28 in reply to:  27 Changed 12 months ago by Michael Orlitzky

Replying to fbissey:

It's good to get rid of that particular patch. What is happening with the "other" patch, namely matrixexp.patch? Is there a ticket for it that escaped my attention?

I don't think I opened a ticket. I did investigate a bit, and found the upstream bug:

https://sourceforge.net/p/maxima/bugs/2596/

The patch was introduced in #13973 to fix a failing doctest. There is apparently a workaround within maxima by enabling "ratmx" for the exponentiation, but I don't think there's a sage interface for that. We could always mark the doctest # known bug and link to the upstream issue.

comment:29 Changed 12 months ago by Volker Braun

Branch: u/mkoeppe/ticket/31553a842fc0d159d9a183f60a6795836a7693ee11a65
Resolution: fixed
Status: positive_reviewclosed

comment:30 Changed 11 months ago by Samuel Lelièvre

Commit: a842fc0d159d9a183f60a6795836a7693ee11a65

I opened #32898 so that removing matrixexp.patch has its own ticket.

Note: See TracTickets for help on using tickets.