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

WIP - port to Cython 3, and GAP 4.12 #24

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

dimpase
Copy link
Contributor

@dimpase dimpase commented May 7, 2024

an attempt to start port to Cython 3 and GAP 4.12 (4.13 hopefully then).

It builds, and import gap works.

Also, in needs work to add stream output, which got broken due to changed functionality in GAP.

@dimpase dimpase marked this pull request as draft May 7, 2024 22:13
@dimpase
Copy link
Contributor Author

dimpase commented May 9, 2024

porting to Pyhton 3.12 has to be done. Easiest would be to use the recent code by @tornaria in sage/fpylll/cypari2.

The layout for python integers changed in python 3.12.
We add a module `gappy.cpython.pycore_long` which copies the new
(internal) api of PyLong from python 3.12. We also implement fallback
version of these functions suitable for python pre-3.12.

Note the files implementing the `pycore_long` module (`pycore_long.pxd`
and `pycore_long.h`) are shared with SageMath, fpylll and cypari2.

cf. sagemath/sage,
 commit dc71bd03c4db0e50cf85b8b3d1cdd4c44b9bd385
 Author: Gonzalo Tornaría <tornaria@cmat.edu.uy>
 Date:   Mon Oct 2 13:20:32 2023 -0300

    py312: changes in PyLong internals

    The layout for python integers changed in python 3.12.
    We add a module `sage.cpython.pycore_long` which copies the new
    (internal) api of PyLong from python 3.12. We also implement fallback
    version of these functions suitable for python pre-3.12.

    Note the files implementing the `pycore_long` module (`pycore_long.pxd`
    and `pycore_long.h`) are shared with fpylll and cypari2.
@embray
Copy link
Owner

embray commented May 9, 2024

Did you see #23 ? It made some fixes that might be relevant here too.

@dimpase
Copy link
Contributor Author

dimpase commented May 9, 2024

yeah, some of #23 repeat what I did, some of it makes no sense.

The output thing needs to use the new GAP kernel function, CALL_WITH_STREAM added in 4.12, see gap-system/gap#2839

It's on my todo list.

@embray
Copy link
Owner

embray commented Jun 2, 2024

Will definitely have a look soon. Just need to get GAP itself set up again

@dimpase
Copy link
Contributor Author

dimpase commented Jun 2, 2024

by the way, in Sage we are getting bitten by the absence of a stateless way to convert Python objects to GAP, to call GAP functions.
(CallWith3Args etc).
see links in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114872 (was resolved invalid, and for a good reason).

Basically one must declare most if not all the Obj created in Sage volatile, otherwise in Pyhton 3.12
one gets segfaults (I guess in earlier Pythons one gets memory leaks)
from longmps in GAP, due to gcc putting Obj data in registers.

@dimpase
Copy link
Contributor Author

dimpase commented Jun 2, 2024

gap-system/gap#5714

@dimpase
Copy link
Contributor Author

dimpase commented Jun 5, 2024

Hi @saraedum - could you explain what's wrong in

Run conda install -q -c conda-forge gap-defaults=4.13.0
Channels:
 - conda-forge
 - defaults
Platform: linux-64
Collecting package metadata (repodata.json): ...working... done
Solving environment: ...working... failed

PackagesNotFoundError: The following packages are not available from current channels:

  - gap-defaults=4.13.0*

Current channels:

  - https://conda.anaconda.org/conda-forge
  - defaults

As far as I can see, conda-forge does have GAP-4.13.0.

@saraedum
Copy link

saraedum commented Jun 5, 2024

Note: 'use_only_tar_bz2' is enabled. This might be omitting some
packages from the index. Set this option to 'false' and retry.

These packages are not in the .tar.bz2 format.

@saraedum
Copy link

saraedum commented Jun 5, 2024

Why is that flag in the workflow file?

@dimpase
Copy link
Contributor Author

dimpase commented Jun 5, 2024

This is the question for whoever put it there

@dimpase
Copy link
Contributor Author

dimpase commented Jun 5, 2024

I'll see what happens if it's removed

@embray
Copy link
Owner

embray commented Jun 5, 2024

According to 3 years ago me "It's apparently required for caching to work properly" b2df390

Which makes a certain sense. But I don't know if we really need it.

@dimpase
Copy link
Contributor Author

dimpase commented Jun 6, 2024

now the tests are running (some failing, as expected, due to incomplete porting)

@dimpase dimpase mentioned this pull request Jun 6, 2024
@embray
Copy link
Owner

embray commented Jun 10, 2024

Would it still make sense to continue supporting GAP 4.10.x via preprocessor macros? Or has the community mostly moved beyond it?

@dimpase
Copy link
Contributor Author

dimpase commented Jun 10, 2024

Would it still make sense to continue supporting GAP 4.10.x via preprocessor macros? Or has the community mostly moved beyond it?

I don't think any non-obsolete distro has GAP 4.10. See https://repology.org/project/gap/versions
So it can be dropped without any problem for anyone.

@embray
Copy link
Owner

embray commented Jun 10, 2024

Ok, just checking. I'm inclined to agree it's not worth the effort at this point, especially if we want to get a working release out.

@dimpase
Copy link
Contributor Author

dimpase commented Jun 10, 2024

I propose to stick to gap 4.12 and 4.13, actually, due to incompatible API changes in libgap, compared to 4.11.

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

Successfully merging this pull request may close these issues.

None yet

3 participants