Merge pynac sources as src/sage/symbolic/ginac
Closed this issue · 158 comments
Pynac has a compile-time dependency on the Python library but is not installed using Python package infrastructure. This is problematic because Python users cannot install it using standard Python tools - for example for testing different Python versions.
Pynac has no other uses than as the core of the symbolic expressions facility of Sage; and cannot even be tested without Sage.
We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as src/sage/symbolic/ginac. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC. (The existing !Python/Cython symbolics code in sage.symbolic, sage.calculus, sage.functions is about 60kLOC.)
The Pynac sources are compiled like other C++ sources that are already in the Sage source tree. They are linked into a single Python extension module, sage.symbolic.expression. All other extension modules that used to link to pynac (sage.libs.pynac.pynac, sage.symbolic.function, sage.symbolic.series etc.) are
- either refactored so that they only
importhelper functions provided bysage.symbolic.expressioninstead of having tocimportPynac functions directly (see #32391, #32407) - or merged into the extension module
sage.symbolic.expression(via Cythonincludestatements, not by copy-paste of source code, to keep the diff small)
This solves the same issues that #30534 tried to address, which ran into unresolved technical difficulties.
It will also make it easier for Sage developers to make changes to Pynac and the related symbolic implementation.
Follow-up: #32387 Remove pynac spkg
Depends on #32391
Depends on #32407
Depends on #32461
CC: @kliem @kiwifb @antonio-rojas @DaveWitteMorris @slel @dimpase @tscrim @egourgoulhon @nbruin
Component: symbolics
Author: Matthias Koeppe, Jonathan Kliem
Branch: 0d0b58f
Reviewer: Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/32386
Description changed:
---
+++
@@ -2,7 +2,7 @@
Pynac has no other uses than as the core of the symbolic expressions facility of Sage; and cannot even be tested without Sage.
-We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as `src/sage/symbolics/ginac`. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC.
+We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as `src/sage/symbolic/ginac`. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC.
This will solve the same issues that #30534 tried to address, which ran into unresolved technical difficulties.
I ran git filter-branch -f --tree-filter 'rm -f Makefile.am; mkdir -p src/sage/symbolic/ginac; mv *.* src/sage/symbolic/ginac/; :' -- --all on pynac 0.7.29 and merged it here, thus preserving the relevant history
Last 10 new commits:
f3bb79f | Merge pull request #374 from slel/fix-exp-inv-hyp |
a99791b | apply patches from #31479 and #31554 |
91cb6fa | apply trac 31869 fix pynac integer_content |
d96393c | apply trac 30688 patch infinite loop in pynac |
80269c5 | apply trac 31645 handling of constant term in series |
4caca2e | apply trac 31530 patch pynac power::subs |
cba9038 | trac 31585 32-bit overflow |
8acb548 | also fix /= |
f2040c8 | ginac/numeric.cpp: Only use builtin for smull_overflow if supported by the compiler |
e43830d | Merge remote-tracking branch 'pynac-filtered/master' into t/32386/merge_pynac_as_src_sage_symbolic_ginac |
Branch pushed to git repo; I updated commit sha1. New commits:
2bbf412 | build/pkgs/pynac: Remove |
Next: Move stuff from sage.libs.pynac to sage.symbolic.ginac, leaving some deprecated reimports behind.
Help from Cython experts very welcome...
Branch pushed to git repo; I updated commit sha1. New commits:
e693827 | Move stuff from sage.libs.pynac to sage.symbolic.ginac, leaving some deprecated reimports behind |
This version compiles OK
Author: Matthias Koeppe
Description changed:
---
+++
@@ -2,7 +2,7 @@
Pynac has no other uses than as the core of the symbolic expressions facility of Sage; and cannot even be tested without Sage.
-We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as `src/sage/symbolic/ginac`. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC.
+We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as `src/sage/symbolic/ginac`. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC. (The existing Python/Cython symbolics code in `sage.symbolic`, `sage.calculus`, `sage.functions` is about 60kLOC.)
This will solve the same issues that #30534 tried to address, which ran into unresolved technical difficulties.
This mostly works but I'm getting a few timeouts/crashes that I'm not sure about
sage -t --random-seed=0 src/sage/schemes/elliptic_curves/ell_number_field.py # Timed out
sage -t --random-seed=0 src/sage/manifolds/differentiable/degenerate_submanifold.py # 1 doctest failed
sage -t --random-seed=0 src/sage/manifolds/differentiable/tensorfield.py # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/parametric_plot3d.py # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/plot3d.py # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/base.pyx # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/implicit_plot3d.py # Timed out
sage -t --random-seed=0 src/sage/symbolic/relation.py # Timed out
sage -t --random-seed=0 src/sage/symbolic/expression.pyx # Killed due to illegal instruction
sage -t --random-seed=0 src/sage/symbolic/pynac_constant.pyx # 18 doctests failed
Branch pushed to git repo; I updated commit sha1. New commits:
d9a1420 | src/sage/symbolic/pynac_constant.pyx: Fix imports in doctests |
The crash is from this doctest:
>>> m = var('m')
>>> assume(m, 'integer')
>>> (I**m).real_part()
which seems to lead to an infinite recursion through GiNaC::power::real_part()
Branch pushed to git repo; I updated commit sha1. New commits:
bc0d5f3 | Trac #28357: patch power::real_part |
Ready for testing and review. (I think the patchbot will not run because build/pkgs/pynac/ is removed by the ticket.)
Branch pushed to git repo; I updated commit sha1. New commits:
881e21b | .gitignore: Unignore ginac cpp sources |
Replying to @mkoeppe:
Ready for testing and review. (I think the patchbot will not run because
build/pkgs/pynac/is removed by the ticket.)
That's a bit inconvenient. Such a massive change needs all the bot runs it can get.
I can of course move the pynac removal to a follow-up ticket
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8c1fd8d | .gitignore: Unignore ginac cpp sources |
c8c3f53 | Trac #28357: patch power::real_part |
e117724 | Move stuff from sage.libs.pynac to sage.symbolic.ginac, leaving some deprecated reimports behind |
e166523 | src/sage/symbolic/ginac/version.h.in: Remove |
3e428de | src/sage/symbolic/ginac/pynac-config.h: New |
dec1c40 | src/sage/symbolic/ginac/utils.h, pynac-config.h: Eliminate use of PYNAC_SIZEOF_LONG, PYNAC_SIZEOF_LONG_LONG |
17d6d2d | src/sage/symbolic/pynac_constant.pyx: Fix imports in doctests |
Description changed:
---
+++
@@ -8,5 +8,5 @@
It will also make it easier for Sage developers to make changes to Pynac.
+Follow-up: #32387 Remove pynac spkg
-My previous tests were on macOS. On Linux this fails with
File "sage/symbolic/pynac.pyx", line 1, in init sage.symbolic.pynac (build/cythonized/sage/symbolic/pynac.cpp:31348)
ImportError: /home/sagemath/sage-9.1/local/lib/python3.7/site-packages/sage/symbolic/expression.cpython-37m-x86_64-linux-gnu.so: undefined symbol: _ZTIN5GiNaC5basicE
(as reported by a patchbot; also verified using tox -e docker-ubuntu-focal-standard)
This problem with linking can likely be solved by merging all *.pyx files that cimport from sage.symbolic.pynac into one (taking care of the ones in ring.pyx by refactoring)
$ git grep pynac.*cimport
src/sage/symbolic/comparison.pyx:from sage.symbolic.pynac cimport (
src/sage/symbolic/expression.pxd:from sage.symbolic.pynac cimport GEx
src/sage/symbolic/expression.pyx:from sage.symbolic.pynac cimport *
src/sage/symbolic/function.pyx:from sage.symbolic.pynac cimport (
src/sage/symbolic/getitem.pyx:from sage.symbolic.pynac cimport GEx
src/sage/symbolic/pynac.pyx:from .pynac_constant cimport PynacConstant
src/sage/symbolic/pynac_constant.pxd:from .pynac cimport GConstant
src/sage/symbolic/pynac_constant.pyx:from .pynac cimport (
src/sage/symbolic/ring.pyx:from sage.symbolic.pynac cimport (GEx, GExprSeq, GExVector, GSymbol,
src/sage/symbolic/series.pyx:from sage.symbolic.pynac cimport GEx, g_is_a_terminating_series, g_series_var, series_to_poly
src/sage/symbolic/substitution_map.pxd:from sage.symbolic.pynac cimport GExMap
src/sage/symbolic/substitution_map.pyx:from sage.symbolic.pynac cimport make_pair, GEx, GExPair
I don't know if there's a better solution.
I will take a look, if I can resolve this.
Branch pushed to git repo; I updated commit sha1. New commits:
70bcddf | simpler inclusion of headers |
Branch pushed to git repo; I updated commit sha1. New commits:
7ce050f | somewhat fix doctest |
I basically have this working by specifying:
diff --git a/src/sage/symbolic/series.pyx b/src/sage/symbolic/series.pyx
index 0e395ae4b4..d93cccc50b 100644
--- a/src/sage/symbolic/series.pyx
+++ b/src/sage/symbolic/series.pyx
@@ -1,3 +1,4 @@
+# distutils: extra_link_args = /srv/public/kliem/sage/local/lib/python3.7/site-packages/sage/symbolic/pynac.cpython-37m-x86_64-linux-gnu.so
"""
Symbolic Seriesto all of those files (and the one doctest in sage/symbolic/pynac.pxd that raises an ImportError currently).
So this is basically working, but the correct linking is missing.
We could create an extra file sage/symbolic/linked_pynac.pxd, which just consists of
# distutils: extra_link_args = ...
from sage.symbolic/pynac cimport *
then there is no need to specify this extra distutils thing in every file that cimports pynac.
Once more thing: The headers in pkgs/sagemath-standard/build/cythonized/ are not cleaned up. Can this be fixed somehow or is this just going to stay this way? (This is probably the reason, why you did not notice the missing headers that were fixed in ||01dfef3||missing headers||
I don't like the change sagemath/sagetrac-mirror@70bcddf (simpler inclusion of headers); putting stuff in the include search path can lead to shadowing headers of other libraries
Thanks for the investigation and fixes.
I think I would be reluctant to use the extra_link_args workaround -- it will requires changes to the build system because now building one extension module would depend on another extension module.
In any case, I'd think that your successful experiment indicates the approach of merging everything that needs to cimport stuff from ginac/ into one extension module would work.
Replying to @kliem:
Once more thing: The headers in
pkgs/sagemath-standard/build/cythonized/are not cleaned up.
Do we already have a ticket for this? If not, please open one
However, I think we should move away from the custom build system of Sage. I have opened #32390 for the next step in this.
Replying to @mkoeppe:
Replying to @mkoeppe:
This problem with linking can likely be solved by merging all *.pyx files that
cimportfromsage.symbolic.pynacinto one (taking care of the ones inring.pyxby refactoring)For cleaning up
ring.pyx, I have opened #32391.
We would still lose ability to use it directly in cython at all. E.g. the doctest in pynac.pxd would fail.
At the moment, I'm building this branch with ./configure --enable-editable and will report whether this resolves anything.
Replying to @mkoeppe:
I don't like the change sagemath/sagetrac-mirror@70bcddf (simpler inclusion of headers); putting stuff in the include search path can lead to shadowing headers of other libraries
from ... cimport ... should usually just work out of the box. But of course one can also spell out all those header (e.g. with a cython alias).
Then again, if we decide to put it all into one large pyx and not cimport things from ginac directly, the commit isn't necessary at all.
Replying to @kliem:
Replying to @mkoeppe:
Replying to @mkoeppe:
This problem with linking can likely be solved by merging all *.pyx files that
cimportfromsage.symbolic.pynacinto one (taking care of the ones inring.pyxby refactoring)For cleaning up
ring.pyx, I have opened #32391.We would still lose ability to use it directly in cython at all. E.g. the doctest in
pynac.pxdwould fail.
I wouldn't be too concerned about this. I'd be surprised if there's any user code in the wild that does that; and if at all necessary, it can be mitigated by adding some missing methods at a higher level.
Ok, same problem with ./configure --enable-editable.
Btw, this is exactly our problem and there is no proper solution to it. So instead of adding a custom one, I agree that it would be easier, to move all the cdef extern from ... into one large pyx and only cimport the cython wrappers. This is the way, your meant to do it anyway.
A simple way of doing this might be to rename files like series.pxd to series.pxi and series.pyx to series_impl.pxi and then to include them into the target extension module (pynac.pxd, pynac.pyx?)
Using pxi is outdated, I was told. But maybe using something outdated is more desirable than having huge files.
That's probably just from the warning at https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html?highlight=include#the-include-statement-and-include-files; but if I'm not mistaken, this just refers to a particular use case of "pxi" that was superseded by "pxd". I don't see any indication that the mechanism of include is intended to be removed in Cython.
(cherry-picked some of your changes onto this branch)
Branch pushed to git repo; I updated commit sha1. New commits:
57ff18a | Make sage.symbolic.expression the extension module that includes pynac |
or perhaps better like this.
Branch pushed to git repo; I updated commit sha1. New commits:
6a2ada2 | src/sage/symbolic/expression.pyx: Move 'distutils: sources' here |
Forgive me for asking a naïve question, but isn't this going against the modularization you are trying to have Sage do? I fully understand the reason for bringing it in (which should make it easier to bug-fix), but doesn't this make Sage more monolithic?
Replying to @tscrim:
isn't this going against the modularization you are trying to have Sage do? I fully understand the reason for bringing it in (which should make it easier to bug-fix), but doesn't this make Sage more monolithic?
This is a natural question.
The pynac library just does not make sense as a module. It is the basis of the implementation of SR elements and as such there is no way to build Sage symbolics without it.
In the modularization plan of #29705, we would instead create a distribution package sagemath-symbolics, containing the integrated pynac from the present ticket and most of sage.symbolic, sage.calculus, sage.functions, and parts of sage.interfaces. This will make sense as a module because there are many parts of Sage that (fortunately!) do not depend at all on Sage symbolics, or only depend on it for some smaller features. For example, sage.graphs has 0 imports from sage.symbolic, sage.combinat just a handful, etc.
Thank you for the explanation. So wouldn't it make sense to instead take the symbolics stuff out from Sage in one go rather than this two step process of add in pynac just to remove it later? Or is this just meant to be a stop-gap until it comes time to split everything?
Replying to @tscrim:
So wouldn't it make sense to instead take the symbolics stuff out from Sage in one go rather than this two step process of add in pynac just to remove it later? Or is this just meant to be a stop-gap until it comes time to split everything?
The modularization plan does not involve "taking ... stuff out from Sage" at all. It will all remain in the same git repository, and also src/sage will remain monolithic.
Modularization is achieved by preparing sub-distributions defined using one of several possible mechanisms. Take a look at sagemath-objects from #29865 (https://pypi.org/project/sagemath-objects/), for example: It defines a sub-distribution using a MANIFEST file. Via the Python 3 "native namespace packages" mechanism, (after some work in #31420, #28925), these distributions can be installed cleanly next to each other and provide the shared sage.* namespace.
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ef22257 | src/sage/symbolic/function.pyx: Undo rename |
4ac54df | src/sage/symbolic/function.pyx: Replace cimport * by more specific cimport |
6f49aee | sage.symbolic.function.Function.__call__: Refactor through new helper function sage.symbolic.expression.call_by_ginac_serial |
2f92b90 | sage.symbolic.function.Function._register_function: Refactor through new function sage.symbolic.expression.register_or_update_function |
e81e6c7 | sage.symbolic.function.GinacFunction._register_function: Refactor through sage.symbolic.expression.register_or_update_function |
d397ef1 | sage.symbolic.function: Remove unused imports, cimports |
1c43dbe | sage.symbolic.function.{GinacFunction,BuiltinFunction}._is_registered: Refactor through new function sage.symbolic.expression.find_registered_function; remove last pynac import |
9d4fe5f | sage.symbolic.function: Remove unused cimport |
9a9fdc7 | Merge #32407 |
759aa5d | sage.symbolic.expression: Finish merging code using pynac into this one DSO |
It remains to remove the import cycle sage.symbolic.ring -> sage.symbolic.expression -> sage.symbolic.function -> sage.symbolic.ring
Ah, okay, I was thinking the modularization meant splitting Sage up into different subprojects that would have a main link that would be the actual Sage. However, it is a little different than that. I don't completely understand how all of this works and some of the benefits, but it doesn't seem to be as fragmented as I was thinking. Thank you.
Branch pushed to git repo; I updated commit sha1. New commits:
903792a | src/sage/symbolic/pynac_constant.py: New |
Description changed:
---
+++
@@ -2,11 +2,15 @@
Pynac has no other uses than as the core of the symbolic expressions facility of Sage; and cannot even be tested without Sage.
-We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as `src/sage/symbolic/ginac`. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC. (The existing Python/Cython symbolics code in `sage.symbolic`, `sage.calculus`, `sage.functions` is about 60kLOC.)
+We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as `src/sage/symbolic/ginac`. It is not a very big package - one flat directory with 100 *.h and *.cpp files, a total of 50kLOC. (The existing !Python/Cython symbolics code in `sage.symbolic`, `sage.calculus`, `sage.functions` is about 60kLOC.)
-This will solve the same issues that #30534 tried to address, which ran into unresolved technical difficulties.
+The Pynac sources are compiled like other C++ sources that are already in the Sage source tree. They are linked into a single Python extension module, `sage.symbolic.expression`. All other extension modules that used to link to pynac (`sage.libs.pynac.pynac`, `sage.symbolic.function`, `sage.symbolic.series` etc.) are
+- either refactored so that they only `import` helper functions provided by `sage.symbolic.expression` instead of having to `cimport` Pynac functions directly (see #32391, #32407)
+- or merged into the extension module `sage.symbolic.expression` (via Cython `include` statements, not by copy-paste of source code, to keep the diff small)
-It will also make it easier for Sage developers to make changes to Pynac.
+This solves the same issues that #30534 tried to address, which ran into unresolved technical difficulties.
+
+It will also make it easier for Sage developers to make changes to Pynac and the related symbolic implementation.
Follow-up: #32387 Remove pynac spkg
Branch pushed to git repo; I updated commit sha1. New commits:
9f56aa2 | sage.symbolic.ring: cimport the helper functions from sage.symbolic.expression (safe because expression.pxd no longer declares the Expression class) |
Branch pushed to git repo; I updated commit sha1. New commits:
3153781 | WIP: Try to break import cycle |
Still struggling with an import cycle sage.symbolic.ring -> sage.symbolic.expression -> sage.symbolic.ring
Help welcome
I recall that i, a.k.a. I, is really close the the center of the import troubles.
Changed author from Matthias Koeppe to Matthias Koeppe, Jonathan Kliem
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9a9fdc7 | Merge #32407 |
759aa5d | sage.symbolic.expression: Finish merging code using pynac into this one DSO |
b640014 | Replace import from sage.symbolic.pynac by sage.symbolic.expression |
46c4a6b | sage.symbolic.function: Move sfunction_serial_dict, get_sfunction_from_serial to .expression, refactor |
903792a | src/sage/symbolic/pynac_constant.py: New |
9f56aa2 | sage.symbolic.ring: cimport the helper functions from sage.symbolic.expression (safe because expression.pxd no longer declares the Expression class) |
3153781 | WIP: Try to break import cycle |
a56c69d | fix startup |
5b557c6 | deprecation warnings |
48267a7 | fix doctests |
Seems to work now.
Branch pushed to git repo; I updated commit sha1. New commits:
5776163 | wrong import |
Cool, thanks very much!!
I'm getting
sage -t --random-seed=0 src/sage/functions/prime_pi.pyx
**********************************************************************
File "src/sage/functions/prime_pi.pyx", line 17, in sage.functions.prime_pi
Failed example:
loads(dumps(z)) == z
Expected:
True
Got:
False