Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade eclib to version 20241112 #38962

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

JohnCremona
Copy link
Member

Fixed #38960

📝 Checklist

  • The title is concise and informative.
  • [x ] The description explains in detail what this PR is about.
  • [ x] I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@JohnCremona
Copy link
Member Author

Anyone looking at this ticket now would assume that there is something wrong with it as several automatic checks have failed. But none of the failures have (as far as I can see) anything to do with the new eclib. I myself am not going to take any further action on this unless someone tells me that I am wrong about this.

@antonio-rojas
Copy link
Contributor

@JohnCremona
Copy link
Member Author

This requires patching sagelib due to API changes. See https://gitlab.archlinux.org/archlinux/packaging/packages/sagemath/-/blob/main/eclib-20241112.patch?ref_type=heads

Thanks. I will make the changes you suggest in the link and update the PR.

Copy link

github-actions bot commented Nov 19, 2024

Documentation preview for this PR (built with commit 8754c70; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@JohnCremona
Copy link
Member Author

The forced push was just after rebasing on current develop.

@JohnCremona
Copy link
Member Author

As usual I have no idea what is making any of the failures, this time I am more confident that it is nothing to do with this PR but whi knows? I hope someone does.

@tobiasdiez
Copy link
Contributor

Only had a quick look but at least the build failures for "Build & Test using Meson" are in the files changed in this PR. I suppose the problem is that these changes are not backwards compatible. Could you make compile sage with both the old and new version of eclib? If not, I'm afraid this PR has to wait until the new version is available on conda.

@JohnCremona
Copy link
Member Author

Only had a quick look but at least the build failures for "Build & Test using Meson" are in the files changed in this PR. I suppose the problem is that these changes are not backwards compatible. Could you make compile sage with both the old and new version of eclib? If not, I'm afraid this PR has to wait until the new version is available on conda.

Thanks for the explanation. I had thought, apparently wrongly, that the code in spkg-configure.m4 which checks for the existence of an installed eclib version which is new enough was there precisely to deal with this situation. If you are building Sage in an environment where only a too-old eclib is available, then the build process will build its own version of eclib. No? That certainly used to be the case. If not, then I don't see how any upstream developer (e.g. me for eclib) can ever reliably upgrade their library for Sage.

@tobiasdiez
Copy link
Contributor

If one uses conda, then all of sage-the-distro (everything under the build folder) is ignored, see https://doc.sagemath.org/html/en/installation/conda.html#using-conda-to-provide-all-dependencies-for-the-sage-library

Is it really not possible to support both the old and new version of ecm in sage?

@JohnCremona
Copy link
Member Author

If one uses conda, then all of sage-the-distro (everything under the build folder) is ignored, see https://doc.sagemath.org/html/en/installation/conda.html#using-conda-to-provide-all-dependencies-for-the-sage-library

Is it really not possible to support both the old and new version of ecm in sage?

(eclib not ecm)

I will try -- there are very few lines which had to be different in the wrapping code.

@JohnCremona
Copy link
Member Author

If one uses conda, then all of sage-the-distro (everything under the build folder) is ignored, see https://doc.sagemath.org/html/en/installation/conda.html#using-conda-to-provide-all-dependencies-for-the-sage-library
Is it really not possible to support both the old and new version of ecm in sage?

(eclib not ecm)

I will try -- there are very few lines which had to be different in the wrapping code.

OK, I can do away with the get_entries() methods which used to return a scalar* and now return a vector[scalar], and use the methods which deliver individual entries.

Incidentally, I would be amazed if there was anyone in the world who actually use this part of eclib in Sage: it is the conversion to a Sage matric f an eclib matrix, as returned by the Hecke operator functions on a CremonaModularSymbols space.

I will update the interface code and update this PR.

@JohnCremona
Copy link
Member Author

OK, I rebased on current develop and made a few changes to the interface so that neither scalar* nor vector[scalar] are used. I hope this is enough.

@JohnCremona
Copy link
Member Author

I checked the logs for failing tests and the only one is in src/sage/libs/gap/element.pyx which is not relevant to this PR, I think. In particular, it was happy with the tests in src/sage/libs/eclib.

@dimpase
Copy link
Member

dimpase commented Dec 4, 2024

still needs a rebase

@JohnCremona
Copy link
Member Author

If someone tells me what is now wrong and how to fix it, and if it is anything to do with my latest attempt to upgrade eclib (which used to be easy enough for me to do) then I will fix it.

@dimpase
Copy link
Member

dimpase commented Dec 5, 2024

lgtm

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 6, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixed sagemath#38960

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x ] The description explains in detail what this PR is about.
- [ x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38962
Reported by: John Cremona
Reviewer(s): Dima Pasechnik
@vbraun
Copy link
Member

vbraun commented Dec 8, 2024

Segfaults on 32-bit:

sage: E = EllipticCurve('11a') ## line 1454 ##
sage: phi = E.pollack_stevens_modular_symbol() ## line 1455 ##
------------------------------------------------------------------------
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/python3.12/site-packages/cysignals/signals.cpython-312-i386-linux-gnu.so(+0x8372)[0xf680a372]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/python3.12/site-packages/cysignals/signals.cpython-312-i386-linux-gnu.so(+0x840f)[0xf680a40f]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/python3.12/site-packages/cysignals/signals.cpython-312-i386-linux-gnu.so(+0xb354)[0xf680d354]
linux-gate.so.1(__kernel_sigreturn+0x0)[0xf7f5c570]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_Z12gauss_reducellllRlS_S_S_+0xda)[0xa4504a5a]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_Z10new_modratllRlS_+0x42)[0xa4504c52]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_Z6modratllRlS_+0x24)[0xa4504cf4]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_Z6modratiiRiS_+0x26)[0xa4504d26]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_Z4liftRK5vec_iRKiRS_+0x2c0)[0xa4525b20]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_Z4liftRK5vec_i+0x41)[0xa4581d01]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_ZN12form_finder211make_basis2ER7ff_dataRK5vec_i+0xd1)[0xa4581ed1]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_ZN12form_finder210make_basisER7ff_data+0x409)[0xa4582399]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_ZN12form_finder28splitoffERKSt6vectorIlSaIlEE+0x386)[0xa4584246]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_ZN12form_finder27recoverESt6vectorIS0_IlSaIlEESaIS2_EE+0x64)[0xa45845c4]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_ZN8newforms16createfromcurvesEiSt6vectorI8CurveRedSaIS1_EEi+0x531)[0xa46dcb01]
/var/lib/buildbot/worker/sage_git/build/local/lib/libec.so.14(_ZN8newforms15createfromcurveEiRK8CurveRedi+0xd7)[0xa46dcf47]
/var/lib/buildbot/worker/sage_git/build/src/sage/libs/eclib/newforms.cpython-312-i386-linux-gnu.so(+0xba36)[0xa479ba36]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/libpython3.12.so.1.0(+0x12da60)[0xf7ae7a60]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/libpython3.12.so.1.0(_PyObject_MakeTpCall+0x9c)[0xf7a7ef2c]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/libpython3.12.so.1.0(PyObject_Vectorcall+0x74)[0xf7a7f264]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/libpython3.12.so.1.0(_PyEval_EvalFrameDefault+0x5025)[0xf7a29cf5]
/var/lib/buildbot/worker/sage_git/build/local/var/lib/sage/venv-python3.12.5/lib/libpython3.12.so.1.0(+0x1b7311)[0xf7b71311]

@tornaria
Copy link
Contributor

See: JohnCremona/eclib#81 (comment)

A new version of modrat() has been introduced, and it seems to have issues with overflow. Definitely for 32 bit, and maybe only for 32 bit, but in the end I just reverted to old_modrat() with https://github.com/tornaria/void-packages/blob/pari/srcpkgs/eclib/patches/fix-32bit-modrat.patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade eclib to version v20241112
6 participants