upgrade lrcalc to 2.1
Closed this issue · 136 comments
lrcalc 2.1 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog
Instead of rewriting Sage's Python bindings to support the new API, we replace them with a wrapper around the new upstream provided bindings
Upstream:
lrcalclib-2.1.tar.gz (lrcalc): https://sites.math.rutgers.edu/~asbuch/lrcalc/lrcalc-2.1.tar.gz
lrcalc-2.1.tar.gz (lrcalc_python): https://pypi.io/packages/source/l/lrcalc/lrcalc-2.1.tar.gz
CC: @antonio-rojas @anneschilling @fchapoton @tscrim @thierry-FreeBSD @orlitzky @mkoeppe @kiwifb @asbuch @slel
Component: packages: optional
Keywords: upgrade, lrcalc
Author: Antonio Rojas, Matthias Koeppe
Branch/Commit: de4fe09
Reviewer: Matthias Koeppe, Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/31355
Description changed:
---
+++
@@ -1,2 +1,2 @@
-lrslib 2.0 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog
+lrcalc 2.0 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog
Branch: u/tmonteil/upgrade_lrcalc_to_2_0
Commit: 850f9bc
I get the following error when doctesting:
ImportError: /opt/sagemath/sage/local/lib/python3.7/site-packages/sage/libs/lrcalc/lrcalc.cpython-37m-x86_64-linux-gnu.so: undefined symbol: st_new
New commits:
850f9bc | #31355 : upgrade lrcalc to 2.0 |
This has major API changes. Large parts of lrcalc.pyx needs to be rewritten because the used functions have been removed or modified.
I see, any help welcome !
The existing spkg-configure.m4 needs to be updated to detect v2.0 (once it actually works), too.
Some information from Anders:
I recently put out a new version of my lrcalc program, much of it is
rewritten, and some of the most important functions are close to twice as fast
as before. Some special cases have also been optimized to run a lot faster.I have included some (pure) Python3 bindings in the repository, so I think
this update will be easy. But I don't understand Sage well enough to do it
myself.
For reference, some pointers:
- Sage's lrcalc package: https://github.com/sagemath/sagetrac-mirror/blob/develop/build/pkgs/lrcalc
- Sage's lrcalc bindings: https://github.com/sagemath/sagetrac-mirror/blob/develop/src/sage/libs/lcalc
The Python bindings are only in the repository and a bit raw, so probably only useful as examples. But I expect they are easier to start from than the .h files.
Changed branch from u/tmonteil/upgrade_lrcalc_to_2_0 to u/arojas/upgrade_lrcalc_to_2_0
Replying to @asbuch:
The Python bindings are only in the repository and a bit raw, so probably only useful as examples. But I expect they are easier to start from than the .h files.
Actually, they are almost equivalent to the current bindings in Sage. The attached branch replaces lrcalc.pyx with a thin wrapper over lrcalc's bindings.
It mostly just translates the output from lrcalc to sage notation. The only function of lrcalc.pyx that isn't straghtforward to port is quantum multiplication: I found no way to recover the q-grading from lrcalc's output, so this is reimplemented in the patch.
The branch needs an update of lrcalc to include python bindings.
New commits:
6596bbd | Replace lrcalc.pyx with a wrapper over lrcalc's own Python bindings |
Replying to @asbuch:
The Python bindings are only in the repository and a bit raw, so probably only useful as examples. But I expect they are easier to start from than the .h files.
Since you are here and I can not find a bug tracker for lrcalc: the setup.py is missing metadata, so the egg for lrcalc gets installed as UNKNOWN-0.0.0.0
If my bindings are used, then that will make it easier to keep things compatible if I make more changes to the C interface. I should be able to keep the Python interface stable, at least up to reordering of output.
Quantum function: I will add a function mult_quantum_pairs that returns a dict from ((partition),d) to integers, you can switch to that if you want.
setup.py: Does the following include the tags you are looking for? Suggestions welcome!
setup(name='lrcalc',
version='2.0.0',
description='Littlewood-Richardson Calculator',
long_description='Littlewood-Richardson Calculator',
url='https://math.rutgers.edu/~asbuch/lrcalc',
author='Anders Skovsted Buch',
license='GPL3',
ext_modules = cythonize([
Extension("lrcalc", ["lrcalc.pyx"],
libraries=["lrcalc"])
])
)
Branch pushed to git repo; I updated commit sha1. New commits:
627192a | Use Sage integers in results |
Replying to @asbuch:
setup.py: Does the following include the tags you are looking for? Suggestions welcome!
setup(name='lrcalc', version='2.0.0', description='Littlewood-Richardson Calculator', long_description='Littlewood-Richardson Calculator', url='https://math.rutgers.edu/~asbuch/lrcalc', author='Anders Skovsted Buch', license='GPL3', ext_modules = cythonize([ Extension("lrcalc", ["lrcalc.pyx"], libraries=["lrcalc"]) ]) )
That works, thanks. Also, although maybe it is a little bit too late for that, it would be good to bump the soversion of the C shared library since the API changed.
Replying to @antonio-rojas:
Replying to @asbuch:
setup.py: Does the following include the tags you are looking for? Suggestions welcome!
setup(name='lrcalc', version='2.0.0', description='Littlewood-Richardson Calculator', long_description='Littlewood-Richardson Calculator', url='https://math.rutgers.edu/~asbuch/lrcalc', author='Anders Skovsted Buch', license='GPL3', ext_modules = cythonize([ Extension("lrcalc", ["lrcalc.pyx"], libraries=["lrcalc"]) ]) )That works, thanks. Also, although maybe it is a little bit too late for that, it would be good to bump the soversion of the C shared library since the API changed.
I changed the version to "2.0.0" and thought that was in line with this page. Please let me know if I am doing something wrong.
Replying to @asbuch:
I changed the version to "2.0.0" and thought that was in line with this page. Please let me know if I am doing something wrong.
You need to change the -version-info in https://bitbucket.org/asbuch/lrcalc/src/master/src/Makefile.am#lines-23
Replying to @antonio-rojas:
Replying to @asbuch:
I changed the version to "2.0.0" and thought that was in line with this page. Please let me know if I am doing something wrong.
You need to change the -version-info in https://bitbucket.org/asbuch/lrcalc/src/master/src/Makefile.am#lines-23
I see, thanks! According to conventions, is that line in Makefile.am independent from this line in configure.am:
AC_INIT([lrcalc],[2.0.0],[asbuch at math rutgers edu])
Branch pushed to git repo; I updated commit sha1. New commits:
52c120f | Use new mult_quantum degrees option in lrcalc 2.1 for quantum multiplication |
Branch pushed to git repo; I updated commit sha1. New commits:
e13f155 | Upgrade lrcalc to 2.1 and install python bindings |
Branch pushed to git repo; I updated commit sha1. New commits:
b2d2844 | Account for the possibility of different degree terms indexed by the same partition |
spkg-configure needs to be updated to check whether the python bindings are installed, and build sage's spkg otherwise.
Changed author from Thierry Monteil to Thierry Monteil, Antonio Rojas
Sorry for the moving target. However, 2.1 should bring only more speed and no new problems.
The mult_quantum() function now takes an optional boolean arguments "degrees" that causes it to replace each key part with (part,d).
Replying to @antonio-rojas:
spkg-configure needs to be updated to check whether the python bindings are installed, and build sage's spkg otherwise.
We cannot use Python packages from the system, see #29023.
An SPKG providing both a library and Python bindings to it should be split into 2 SPKGs.
Anders, the Python package should declare its build dependencies (in particular Cython) by putting a pyproject.toml file next to the setup.py; see https://snarky.ca/what-the-heck-is-pyproject-toml/
Replying to @mkoeppe:
Replying to @antonio-rojas:
spkg-configure needs to be updated to check whether the python bindings are installed, and build sage's spkg otherwise.
We cannot use Python packages from the system, see #29023.
An SPKG providing both a library and Python bindings to it should be split into 2 SPKGs.
Then maybe sage shouldn't support using system lrcalc 2.x for now, the bindings and the C library should obviously be kept at the same version.
In any case, the sagelib side should be ready now, please someone else take care of the build side.
Whether we support system lrcalc or not, non-Python and Python installations need to be split into two separate SPKGs (they can share the same tarball).
Is this the pyproject.toml that you need? If so, then I can add that.
[build-system]
requires = ["setuptools", "cython"]
build-backend = "setuptools.build_meta"
Regarding splitting into SPKGs, I am not familiar with the details of Python's package system. Maybe one day I will read up on this and make it possible to say pip install lrcalc, but this is not likely to happen soon. One way to proceed for now is to copy my files liblrcalc.pxd and lrcalc.pyx to the Sage repository next to Nicolas' files. Changes to liblrcalc's interface are not likely to happen soon, but even if that happened, Sage could be brought up-to-date with another copy command. Not perfect, but maybe still a small improvement to the way lrcalc has been integrated in the past?
Replying to @asbuch:
Is this the
pyproject.tomlthat you need? If so, then I can add that.[build-system] requires = ["setuptools", "cython"] build-backend = "setuptools.build_meta"
Yes, that's good; best to add "wheel" in requires too.
Regarding splitting into SPKGs, I am not familiar with the details of Python's package system.
This is actually Sage-specific, not related to Python packaging, so not a task for you as the upstream author.
"Best practices" on your side would be to create a source distribution for the Python library, using setup.py sdist and to upload it to PyPI. On the Sage packaging side, we can then use that as our upstream for a lrcalc_python SPKG.
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.
Trying to move this forward
Description changed:
---
+++
@@ -1,2 +1,3 @@
-lrcalc 2.0 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog
+lrcalc 2.1 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog
+Instead of rewriting Sage's Python bindings to support the new API, we replace them with a wrapper around the new upstream provided bindings-1 on using a Python package that is not on PyPI
Setting a new milestone for this ticket based on a cursory review.
Replying to @mkoeppe:
-1 on using a Python package that is not on PyPI
we have to move forward here, still. So we can either use the non-PyPI package, or facilitate
a release on PyPI...
It would block the Sage library (sagemath-standard) from being pushed to PyPI, so no, that's not a viable approach.
Replying to @dimpase:
facilitate a release on PyPI...
Facilitated now by providing this link: https://realpython.com/pypi-publish-python-package/#publishing-to-pypi
Someone would probably need to provide a PR for upstream, not just an RTFM link.
No PR is needed, the upstream maintainer really just needs to type python3 setup.py sdist && twine upload dist/*
Changed keywords from none to upgrade, lrcalc
Cc-ing the lrcalc maintainer (hi Anders!).
I would like to have lrcalc available on PyPI, but I have not had time to figure out how to put it there. I don't know how to make sure the Python bindings can find the C library, but if PyPI has some magic that can handle that with the above command, then great. I tried this:
python3 setup.py sdist
twine upload --repository testpypi dist/*
That appeared to work, and resulted in https://test.pypi.org/project/lrcalc/ appearing on TestPyPI. However, when I use the pip command displayed there, I get a lot of errors. I'm not eager to have a defunct project in public view on PyPI.
All that said, I can't help thinking that if Sage was able to include the previous version of lrcalc that did not come with Python bindings, but is unable to include the new version, with or without using the supplied bindings, then something in the project must have gotten less flexible in the last few years.
Changed branch from u/arojas/upgrade_lrcalc_to_2_0 to u/mkoeppe/upgrade_lrcalc_to_2_0
Branch pushed to git repo; I updated commit sha1. New commits:
0bb238b | build/pkgs/lrcalc_python/spkg-install.in: Fixup for sdist layout |
Replying to @asbuch:
All that said, I can't help thinking that if Sage was able to include the previous version of lrcalc that did not come with Python bindings, but is unable to include the new version, with or without using the supplied bindings, then something in the project must have gotten less flexible in the last few years.
The thing is that the API changes in lrcalc 2 require an almost complete rewrite of sage's interface, which would essentially make it a duplicate of your python bindings, and doing that doesn't make much sense to me.
Another solution would be to include a copy of your pyx in sage, if the licence allows it.
Branch pushed to git repo; I updated commit sha1. New commits:
fabfb2b | build/pkgs/lrcalc_python/package-version.txt: 2.0.0 |
Replying to @asbuch:
I don't know how to make sure the Python bindings can find the C library
No need to worry about this. It is the responsibility of the user to install the C library. This is normal.
But it would be a good idea to explain this in the Python package's README (which would appear in the "Project description" at https://test.pypi.org/project/lrcalc/#description)
See https://pypi.org/project/pplpy/ for inspiration.
In lrcalc/python, probably best to change README.Python to either README.md or README.rst
The file on TestPyPI does not include the .pyx and .pxd files but it ships a generated .c file that is not needed. To fix this, you can create a file MANIFEST.in
Replying to @antonio-rojas:
Replying to @asbuch:
All that said, I can't help thinking that if Sage was able to include the previous version of lrcalc that did not come with Python bindings, but is unable to include the new version, with or without using the supplied bindings, then something in the project must have gotten less flexible in the last few years.
The thing is that the API changes in lrcalc 2 require an almost complete rewrite of sage's interface, which would essentially make it a duplicate of your python bindings, and doing that doesn't make much sense to me.
Another solution would be to include a copy of your pyx in sage, if the licence allows it.
This is what I imagined would happen when I wrote the Python bindings. I think I also suggested it at some point. Lrcalc is GPL v3, is Sage not compatible with that?
The Sage library is "GPL v2 or later", I don't think we can put a GPL v3-only module in.
If you relicense the Python bindings to GPL v2 or later, people here on the ticket can probably adapt your code and put it in the Sage library; in this case, we don't need to bother you with Python packaging details.
Replying to @mkoeppe:
The file on TestPyPI does not include the
.pyxand.pxdfiles but it ships a generated.cfile that is not needed. To fix this, you can create a fileMANIFEST.in
Thanks, this is helpful. Do you know a place where I can find a minimal MANIFEST.in to modify?
https://gitlab.com/videlec/pplpy/-/blob/master/MANIFEST.in is a good example
Replying to @mkoeppe:
The Sage library is "GPL v2 or later", I don't think we can put a GPL v3-only module in.
You can hereby consider any files you need to copy as also licensed "GPL v2 or later". Is this official enough?
Replying to @mkoeppe:
https://gitlab.com/videlec/pplpy/-/blob/master/MANIFEST.in is a good example
Thanks, I will give that a try then.
Replying to @asbuch:
Replying to @mkoeppe:
The file on TestPyPI does not include the
.pyxand.pxdfiles but it ships a generated.cfile that is not needed. To fix this, you can create a fileMANIFEST.inThanks, this is helpful. Do you know a place where I can find a minimal MANIFEST.in to modify?
Now the .pyx and .pxd files are included and the .c file is gone, but I get the same result when I try to install with pip. I have liblrcalc installed in /usr/local/lib.
Branch pushed to git repo; I updated commit sha1. New commits:
642747a | build/pkgs/lrcalc_python: Use 2.0.1 from TestPyPI |
Works fine now here on the branch
Replying to @asbuch:
I get the same result when I try to install with pip. I have liblrcalc installed in /usr/local/lib.
If you post some logs, I can take a look
Branch pushed to git repo; I updated commit sha1. New commits:
482bf57 | build/pkgs/lrcalc_python/SPKG.rst: Fix title |
Work Issues: Switch to pypi.io URL when released
Replying to @mkoeppe:
Replying to @asbuch:
I get the same result when I try to install with pip. I have liblrcalc installed in /usr/local/lib.
If you post some logs, I can take a look
Thanks! I created a new Python environment with "mkvirtualenv lrcalc_test", then I tried to install with pip and got the following result:
> pip install -i https://test.pypi.org/simple/ lrcalc
Looking in indexes: https://test.pypi.org/simple/
Collecting lrcalc
Downloading https://test-files.pythonhosted.org/packages/d4/c8/c3988b0714fd225cdc4664f79b57161b8292c43eaad3ff81698d3db58147/lrcalc-2.0.1.tar.gz (15 kB)
Installing build dependencies ... error
ERROR: Command errored out with exit status 1:
command: /home/user/lib/virtualenvs/lrcalc_test/bin/python /home/user/lib/virtualenvs/lrcalc_test/lib/python3.8/site-packages/pip install --ignore-installed --no-user --prefix /tmp/pip-build-env-cz4e5gzo/overlay --no-warn-script-location --no-binary :none: --only-binary :none: -i https://test.pypi.org/simple/ -- setuptools cython wheel
cwd: None
Complete output (3 lines):
Looking in indexes: https://test.pypi.org/simple/
ERROR: Could not find a version that satisfies the requirement setuptools (from versions: none)
ERROR: No matching distribution found for setuptools
----------------------------------------
ERROR: Command errored out with exit status 1: /home/user/lib/virtualenvs/lrcalc_test/bin/python /home/user/lib/virtualenvs/lrcalc_test/lib/python3.8/site-packages/pip install --ignore-installed --no-user --prefix /tmp/pip-build-env-cz4e5gzo/overlay --no-warn-script-location --no-binary :none: --only-binary :none: -i https://test.pypi.org/simple/ -- setuptools cython wheel Check the logs for full command output.
When you use -i, it overrides the default package index URL, so it cannot find setuptools any more. (It does not matter that setuptools is already installed in your venv -- this is Python's build isolation.)
You can use --extra-index-url instead of -i.
Replying to @mkoeppe:
You can use
--extra-index-urlinstead of-i.
That made a difference, thanks! I hope this problem will go away if I post to PyPI? I assume the PyPI website will show the command "pip install lrcalc"?
Replying to @asbuch:
I hope this problem will go away if I post to PyPI? I assume the PyPI website will show the command "pip install lrcalc"?
That's right.
Here it is: https://pypi.org/project/lrcalc/
Many thanks to Matthias for all the help with this!
Changed work issues from Switch to pypi.io URL when released to none
There's one more detail in the actual C library lrcalc. In the lib..._la_LDFLAGS in Makefile.am, it would be good to add the flag -no-undefined - similar to this change: dimpase/autocliquer@ea653f1
This will enable building a shared library on the Cygwin platform (see #29152, #30814).
Also the tarballs for the library and the Python sdist have the same name. I have already worked around this on the Sage side by having the library's tarball renamed, so no action is needed. But it might be a good idea to avoid this potential confusion.
Reviewer: Matthias Koeppe
Replying to @mkoeppe:
Also the tarballs for the library and the Python sdist have the same name. I have already worked around this on the Sage side by having the library's tarball renamed, so no action is needed. But it might be a good idea to avoid this potential confusion.
Thanks for the comments! I added the -no-undefined flag. I may try to change the PyPI tarball name to lrcalc-bindings-x.y.z.tar.gz next time I need to make changes. (Maybe there is an option to setuptools to make this happen automagically?)
I am not aware of an option to customize it in setuptools, I think it's a fixed format that uses the distribution package's name (lrcalc). Maybe easier to rename the C library package in configure.ac
Replying to @mkoeppe:
I am not aware of an option to customize it in
setuptools, I think it's a fixed format that uses the distribution package's name (lrcalc). Maybe easier to rename the C library package in configure.ac
Ok, thanks. In that case I will see if it works if I change the name of the .tar.gz before I upload it with twine. I don't want to change the name I have used for the main package for many years. If the name conflict is a problem, then I would rather change the name of the PyPI project.
Changed author from Thierry Monteil, Antonio Rojas to Antonio Rojas
Needs a rebase :/
Branch pushed to git repo; I updated commit sha1. New commits:
7a0bf0a | Merge tag '9.5.beta2' into t/31355/upgrade_lrcalc_to_2_0 |