search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Second instance of MaximaLib #30097

Open mkoeppe opened 4 years ago

mkoeppe commented 4 years ago

Maxima essentially just uses a single Common Lisp package :MAXIMA.

After loading Maxima into ECL at the beginning of sage.interfaces.maxima_lib, we just rename the package :MAXIMA to :MAXIMA_LIB and remove maxima from *MODULES*.

Because maxima_lib.py is written in a style using lots of global variables, we duplicate this module using a symlink. (This can be improved in a follow-up ticket.) Importing sage.interfaces.maxima_lib_2 loads another copy of Maxima using REQUIRE, and renames the package :MAXIMA to :MAXIMA_LIB_2.

The global name maxima is now imported from sage.interfaces.maxima_lib_2 instead of sage.interfaces.maxima, so it uses the 2nd copy of the maxima library interface.

The two copies of maxima are isolated from each other::

sage: maxima("xyz: 123")
123
sage: maxima_calculus("xyz")
xyz
sage: maxima("xyz")
123

The global name Maxima still refers to the pexpect interface class.

Details:

We make :COMMON-LISP-USER the default package. During maxima_eval, we bind *PACKAGE* to the correct package object. We replace the use of the #$ read macro by a direct use of MREAD (while binding *PACKAGE). During $LOAD, we temporarily set up a package nickname :MAXIMA so that the loaded Lisp files can do (IN-PACKAGE :MAXIMA).

Depends on #30105

CC: @dimpase @nbruin @jpflori @EmmanuelCharpentier

Component: interfaces

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/several_instances_of_maximalib @ a3b58ad

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

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -24,3 +24,9 @@
 # loaded another copy!

+In maxima_eval, we bind *PACKAGE* to the correct package object. We replace the use of the #$ read macro by a direct use of MACSYMA-READ-STRING (from the correct package). + +Because maxima_lib.py is written in a style using lots of global variables, we duplicate this module using a symlink. This can be improved in a follow-up ticket. + + +

mkoeppe commented 4 years ago

Branch: u/mkoeppe/several_instances_of_maximalib

mkoeppe commented 4 years ago

Commit: 177e903

mkoeppe commented 4 years ago

New commits:

177e903sage.interfaces.maxima_lib: Rename the MAXIMA Lisp package
mkoeppe commented 4 years ago

Author: Matthias Koeppe

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

563aeb6Patch $LOAD, INTERN-INVERT-CASE, $GENSYM
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 177e903 to 563aeb6

mkoeppe commented 4 years ago
comment:5

Unfinished, setting it to needs-review so that the patchbot runs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 563aeb6 to 9d4dc92

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

9d4dc92Fix maxima_read_eval, use it for init_code
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

08318dfGet rid of some #$
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 9d4dc92 to 08318df

mkoeppe commented 4 years ago
comment:8

More work to be done, but some preliminary testing makes sense

dimpase commented 4 years ago
comment:9

Are you planning to use this to switch SR to maxima_lib ?

mkoeppe commented 4 years ago
comment:10

SR is already using maxima_lib.

mkoeppe commented 4 years ago
comment:11

I hope to switch the remaining uses of maxima, mostly in doctests, to the second copy of maxima_lib when this ticket is ready, so that we can retire maxima_pexpect.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 08318df to bd555f5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

fb16688: Make more robust: remove a possible nickname MAXIMA, use UNWIND-PROTECT to clean up
bd555f5At the end of loading the module, switch back to :COMMON-LISP-USER
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

e8672a4sage.interfaces.all: Import maxima from .maxima_lib_2
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from bd555f5 to e8672a4

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,32 +1,24 @@
 Maxima essentially just uses a single Common Lisp package `:MAXIMA`.

-After loading Maxima, we just rename the package and remove maxima from `*MODULES*`
-Then we can load another copy.
+After loading Maxima, we just rename the package and remove maxima from `*MODULES*` Then we can load another copy.
+
+Because `maxima_lib.py` is written in a style using lots of global variables, we duplicate this module using a symlink.  (This can be improved in a follow-up ticket.)
+
+The global name `maxima` is now imported from `sage.interfaces.maxima_lib_2` (the second copy, which operates in the Common Lisp package `MAXIMA_LIB_2`.
+
+The two copies of maxima are isolated from each other::

-sage: from sage.interfaces.maxima_lib import ecl_eval -sage: var('y', domain='real') -y -sage: ecl_eval('''(symbol-package '$besselexpand)''') -<ECL: #<"MAXIMA" package>> -sage: ecl_eval('''(rename-package 'maxima "MINIMA")''') -<ECL: #<"MINIMA" package>> -sage: ecl_eval('''(symbol-package '$besselexpand)''') -<ECL: #<"MINIMA" package>> -sage: ecl_eval('''(require 'maxima)''') - -sage: ecl_eval('''modules''') -<ECL: ("CMP" "SB-BSD-SOCKETS" "SOCKETS" "MAXIMA" "ECL-CDB" "BYTECMP")> -sage: ecl_eval('''(delete "MAXIMA" modules :test #'string=)''') -<ECL: ("CMP" "SB-BSD-SOCKETS" "SOCKETS" "ECL-CDB" "BYTECMP")> -sage: ecl_eval('''(require 'maxima)''') -<ECL: ("MAXIMA")> -# loaded another copy! -``` +sage: maxima("xyz: 123") +123 +sage: maxima_calculus("xyz") +xyz +sage: maxima("xyz") +123

In maxima_eval, we bind *PACKAGE* to the correct package object. We replace the use of the #$ read macro by a direct use of MACSYMA-READ-STRING (from the correct package).

-Because maxima_lib.py is written in a style using lots of global variables, we duplicate this module using a symlink. This can be improved in a follow-up ticket.

+```

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,10 +1,11 @@
 Maxima essentially just uses a single Common Lisp package `:MAXIMA`.

-After loading Maxima, we just rename the package and remove maxima from `*MODULES*` Then we can load another copy.
+After loading Maxima into ECL at the beginning of `sage.interfaces.maxima_lib`, we just rename the package `:MAXIMA` to `:MAXIMA_LIB` and remove maxima from `*MODULES*`. 

 Because `maxima_lib.py` is written in a style using lots of global variables, we duplicate this module using a symlink.  (This can be improved in a follow-up ticket.)
+Importing `sage.interfaces.maxima_lib_2` loads another copy of Maxima using `REQUIRE`, and renames the package `:MAXIMA` to `:MAXIMA_LIB_2`.

-The global name `maxima` is now imported from `sage.interfaces.maxima_lib_2` (the second copy, which operates in the Common Lisp package `MAXIMA_LIB_2`.
+The global name `maxima` is now imported from `sage.interfaces.maxima_lib_2` instead of `sage.interfaces.maxima`, so it uses the 2nd copy of the maxima library interface.

 The two copies of maxima are isolated from each other::

@@ -15,10 +16,12 @@
 xyz
 sage: maxima("xyz")
 123
+```

-In `maxima_eval`, we bind `*PACKAGE*` to the correct package object. We replace the use of the `#$` read macro by a direct use of `MACSYMA-READ-STRING` (from the correct package).
+The global name `Maxima` still refers to the pexpect interface class.
+
+Details:
+
+We make `:COMMON-LISP-USER` the default package. During `maxima_eval`, we bind `*PACKAGE*` to the correct package object. We replace the use of the `#$` read macro by a direct use of `MREAD` (while binding `*PACKAGE`). During `$LOAD`, we temporarily set up a package nickname `:MAXIMA` so that the loaded Lisp files can do `(IN-PACKAGE :MAXIMA)`. 

-
-
-```
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

9353d6aRobustness: Make sure MAXIMA is not in *MODULES* at the beginning
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from e8672a4 to 9353d6a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 9353d6a to 0ded2ff

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

0ded2ffRemove remaining uses of the read macro
mkoeppe commented 4 years ago
comment:19

Ready for testing/review

mkoeppe commented 4 years ago
comment:20

Some details still need to be fixed: 

File "src/sage/interfaces/maxima_lib_2.py", line 1509, in sage.interfaces.maxima_lib_2.dummy_integrate
Failed example:
    f.ecl()
Expected:
    <ECL: ((%INTEGRATE SIMP) (($F SIMP) $X) $X)>
Got:
    <ECL: ((MAXIMA_LIB::%INTEGRATE MAXIMA_LIB::SIMP)
     ((MAXIMA_LIB::$F MAXIMA_LIB::SIMP) MAXIMA_LIB::$X) MAXIMA_LIB::$X)>
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

bcd7903Add HANDLER-CASE to MAXIMA-EVAL as a debugging aid
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 0ded2ff to bcd7903

nbruin commented 4 years ago
comment:22

It's nice you can do it, but do we need it? I suspect that the remaining uses of the maxima interface in library code don't actually need to be isolated from the calculus instance.

If you do think it's worth supporting multiple instances of maxima_lib interfaces, then I think it shouldn't be hardcoded at only 2 copies. I think it's possible (with some hackery) to have multiple instances of the same module in python:

https://stackoverflow.com/questions/11170949/how-to-make-a-copy-of-a-python-module-at-runtime

the trick would now be to somehow parametrize the name of the corresponding package in ecl, but that should be quite solvable (if all else fails, we could park a global counter somewhere outside the maxima_lib package -- for instance in ECL if all else fails -- that it can use to see how many copies already exist)

A thing to be careful about it that in CL, at least for the REPL, there is global state as to what the "current package" is. I'm not 100% sure that this isn't somewhere relied upon for maxima.

If we're going down this route, you probably want reconsider the placement of some of the initialization code too. I think there might be some things in maxima_lib that prime it towards "calculus" use. Having a "vanilla" maxima_lib interface would be one of the uses I could see.

mkoeppe commented 4 years ago
comment:23

Replying to @nbruin:

It's nice you can do it, but do we need it? I suspect that the remaining uses of the maxima interface in library code don't actually need to be isolated from the calculus instance.

That's a good point and certainly worth checking; I hope we can find and change such uses as part of #30071.

But there are lots of uses of maxima in doctests too. If we switch these ones over to maxima_calculus, this might be advertising potentially unsafe uses to users.

And the doctests are what have prompted this ticket -- we currently have problems with the Maxima pexpect on Cygwin on #22191 (update ECL to 20.4.24).

Let's have this discussion -- on concrete uses of maxima -- on #30071.

mkoeppe commented 4 years ago
comment:24

Replying to @nbruin:

If you do think it's worth supporting multiple instances of maxima_lib interfaces, then I think it shouldn't be hardcoded at only 2 copies. I think it's possible (with some hackery) to have multiple instances of the same module in python:

https://stackoverflow.com/questions/11170949/how-to-make-a-copy-of-a-python-module-at-runtime

Instead of doing this, I think it would be better to rewrite the Python module so that the module globals are placed in the interface object instead. Trivial to do, but a huge patch, which is why I haven't done this on this ticket.

A thing to be careful about it that in CL, at least for the REPL, there is global state as to what the "current package" is. I'm not 100% sure that this isn't somewhere relied upon for maxima.

Yes, my current code is taking care of this already. There are just some minor details remaining.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from bcd7903 to e4ba567

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

e4ba567Rename also the :BIGFLOAT, :BIGFLOAT-IMPL packages
mkoeppe commented 4 years ago

Dependencies: #30105

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

70a6351sage.libs.ecl.EclObject.__init__: Add optional argument read_strings
70082c6Merge branch 't/30105/sage_libs_ecl__add_python_function_ecl_make_string' into t/30097/several_instances_of_maximalib
0df4595src/sage/interfaces/maxima_lib.py: Use new conversion to Lisp strings
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from e4ba567 to 0df4595

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 0df4595 to a3b58ad

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

a3e0ecaadd upstream fix from MR 215
89b006badd the patch from upstream MR 214
0b77737add upstream MR 216 (to fix cygwin fork)
8ca1c0eMerge tag '9.2.beta2' into t/22191/public/packages/ecl20
f82c716Commit 75877dd8 from upstream
2d87ee3Merge commit 'f82c716fdf9c6e91a07166d36b6329a15ecfb41d' of git://trac.sagemath.org/sage into t/30106/sage_libs_ecl__fix_unicode_handling
828a727Add doctests
0bfaccfMerge branch 't/30106/sage_libs_ecl__fix_unicode_handling' into t/30105/sage_libs_ecl__add_python_function_ecl_make_string
dd27b33sage.libs.ecl: Update uses of python_to_ecl
a3b58adMerge branch 't/30105/sage_libs_ecl__add_python_function_ecl_make_string' into t/30097/several_instances_of_maximalib
dimpase commented 4 years ago
comment:29

there are things like

sage -t --long --warn-long 177.3 src/doc/en/constructions/plotting.rst
**********************************************************************
File "src/doc/en/constructions/plotting.rst", line 216, in doc.en.constructions.plotting
Failed example:
    maxima.eval('load("plotdf");')
Expected:
    '".../share/maxima/.../share/dynamics/plotdf.lisp"'
Got:
    '\\"/home/scratch2/dimpase/sage/sage/local/share/maxima/5.42.2/share/dynamics/plotdf.lisp\\"'
**********************************************************************

and LISP errors such as

File "src/sage/symbolic/expression.pyx", line 12389, in sage.symbolic.expression.Expression.integral
Failed example:
    sin(x).integral(x)
Exception raised:
    Traceback (most recent call last):
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/doctest/forker.py", line 707, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/doctest/forker.py", line 1131, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.expression.Expression.integral[1]>", line 1, in <module>
        sin(x).integral(x)
      File "sage/symbolic/expression.pyx", line 12437, in sage.symbolic.expression.Expression.integral (build/cythonized/sage/symbolic/expression.cpp:64526)
        return integral(self, *args, **kwds)
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/symbolic/integration/integral.py", line 963, in integrate
        return indefinite_integral(expression, v, hold=hold)
      File "sage/symbolic/function.pyx", line 1138, in sage.symbolic.function.BuiltinFunction.__call__ (build/cythonized/sage/symbolic/function.cpp:12274)
        res = super(BuiltinFunction, self).__call__(
      File "sage/symbolic/function.pyx", line 602, in sage.symbolic.function.Function.__call__ (build/cythonized/sage/symbolic/function.cpp:7051)
        res = g_function_eval2(self._serial, (<Expression>args[0])._gobj,
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/symbolic/integration/integral.py", line 117, in _eval_
        A = integrator(f, x)
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/symbolic/integration/external.py", line 44, in maxima_integrator
        result = maxima.sr_integral(expression,v)
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/interfaces/maxima_lib.py", line 877, in sr_integral
        return max_to_sr(maxima_eval(([max_integrate],[sr_to_max(SR(a)) for a in args])))
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/interfaces/maxima_lib.py", line 1763, in max_to_sr
        sage_expr=SR(maxima(expr))
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/interfaces/interface.py", line 303, in __call__
        result = self._coerce_from_special_method(x)
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/interfaces/maxima_lib.py", line 472, in _coerce_from_special_method
        return MaximaLibElement(self,self._create(x))
      File "/home/scratch2/dimpase/sage/sage/local/lib64/python3.7/site-packages/sage/interfaces/maxima_lib.py", line 690, in _create
        maxima_eval([[msetq], maxima_read(name), value])
      File "sage/libs/ecl.pyx", line 852, in sage.libs.ecl.EclObject.__call__ (build/cythonized/sage/libs/ecl.c:8507)
        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:5835)
        raise RuntimeError("ECL says: {}".format(
    RuntimeError: ECL says: In form
    ((MAXIMA_LIB_2::%COS MAXIMA_LIB_2::SIMP) MAXIMA_LIB::|$_SAGE_VAR_x|)
    FUNCTION: Not a valid argument (MAXIMA_LIB_2::%COS MAXIMA_LIB_2::SIMP).
mkoeppe commented 3 years ago
comment:32

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

mkoeppe commented 3 years ago
comment:34

Setting a new milestone for this ticket based on a cursory review.