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

does not work with Python 3.11 #660

Closed
dimpase opened this issue Mar 16, 2023 · 15 comments
Closed

does not work with Python 3.11 #660

dimpase opened this issue Mar 16, 2023 · 15 comments

Comments

@dimpase
Copy link

dimpase commented Mar 16, 2023

On Linux:

  src/pyscipopt/scip.c:463:62: error: invalid use of incomplete typedef 'PyFrameObject' {aka 'struct _frame'}
    463 |   #define __Pyx_PyFrame_SetLineNumber(frame, lineno)  (frame)->f_lineno = (lineno)

this is due to PyFrameObject structure members have been removed from the public C API, cf
https://docs.python.org/3.11/whatsnew/3.11.html

No surprise, as PyFrameObject was always subject to change.

See also https://groups.google.com/d/msgid/sage-devel/42199586-647d-4dc0-9b88-9ff5c7ff8737n%40googlegroups.com.

@dimpase
Copy link
Author

dimpase commented Mar 16, 2023

pyscipopt-4.2.0.log

@mattmilten
Copy link
Collaborator

This may be fixed in a future version of cython. It's not something we can fix in PySCIPOpt.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 17, 2023

Actually @mattmilten (sagemath/sage#35299 (comment)): the PyPI sdist of PySCIPOpt contains a scip.c that was prebuilt with an old Cython. Depending on timestamps when unpacking the sdist, this will not be recythonized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 17, 2023

The solution: Merge #652, tag a new release....

@dimpase
Copy link
Author

dimpase commented Mar 17, 2023

Actually @mattmilten (sagemath/sage#35299 (comment)): the PyPI sdist of PySCIPOpt contains a scip.c that was prebuilt with an old Cython. Depending on timestamps when unpacking the sdist, this will not be recythoniz

these 🥩🍄🔪☢️☠️⚰️ 🥩🍄🔪☢️☠️⚰️ 🥩🍄🔪☢️☠️⚰️ 🥩🍄🔪☢️☠️⚰️
at Cython who still tell ppl to distribute outdated *.c files - @robertwb

@dimpase
Copy link
Author

dimpase commented Mar 17, 2023

This may be fixed in a future version of cython. It's not something we can fix in PySCIPOpt.

it is fixed in our patched Cython 0.29.33, see sagemath/sage#35084 or https://github.com/dimpase/cython/tree/0.29.33.py311patch

In order to build the latest pyscipopt with Python 3.11, I had to manually remove scip.c from the tarball,
because your setup prefers using it over running Cython.

Please make a new release, without scip.c.

Or you can just modify the current release tarball on PyPI and put it to Assets in
https://github.com/scipopt/PySCIPOpt/releases/tag/v4.2.0 (to do this, click on "Edit release" and upload the file)
This is something we would be able to use in Sage

@dimpase
Copy link
Author

dimpase commented Mar 17, 2023

Even better, after removing scip.c from the tarball, I can build with unpatched Cython (0.29.33), or with Sage's
patched Cython 0.29.32.

So there is nothing to fix in Cython, the ball is in your court.

@mattmilten
Copy link
Collaborator

Should be fixed now. Thanks a lot for pushing for this - this release was long overdue!

@dimpase
Copy link
Author

dimpase commented Mar 17, 2023

Your new release tarball on PyPI still contains the same generated C file. Even though it might be out of the woods for the Python 3.11, it probably means trouble down the road, just as well what we saw with 4.2.0.

Besides, it takes over 90% of the tarball, it's whooping 7.6MB (uncompressed).

@dimpase
Copy link
Author

dimpase commented Mar 17, 2023

@mattmilten - would you accept a PR streamlining the setup.py, so that the C file is only generated when the extension is built
(and so it would not go to the tarball) ?

@mattmilten
Copy link
Collaborator

Hmm, no, we did it this way so Cython is no dependency when installing the package.

@dimpase
Copy link
Author

dimpase commented Mar 18, 2023 via email

@mattmilten
Copy link
Collaborator

Sorry, I don't see the advantage. This should also save some compilation time on the user side. And if people want to recompile, they can do that with a clone of the source package.

@dimpase
Copy link
Author

dimpase commented Mar 18, 2023

Compared to the time it takes to compile the C file, Cython takes perhaps 1% of it. Also, downloading bigger tarballs, and expanding them takes extra time.

The main advantage is that you are not sending the user down the garden path, like you did with 4.2 and Python 3.11.

Python 3.12 will come with more API changes, i.e. your C files will likely get outdated again.

Please respect your users and do not subject them to unnecessary pain of discovering that the package doesn't build, because you poison your tarballs. And no, PyPI is really the default place for the tarballs, and they better be clean.

@mattmilten
Copy link
Collaborator

OK, then, you convinced me 😄
If you already know how to implement this in the best way in the setup.py, I will happily merge your PR.

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

No branches or pull requests

3 participants