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

Python 3.11 #369

Merged
merged 4 commits into from Oct 28, 2022
Merged

Python 3.11 #369

merged 4 commits into from Oct 28, 2022

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Oct 27, 2022

Python 3.11 raises a few errors in numcodecs/_shuffle.c that are documented in What’s New In Python 3.11:

  • PyCode_New() and PyCode_NewWithPosOnlyArgs() now take an additional exception_table argument. Using these functions should be avoided, if at all possible. To get a custom code object: create a code object using the compiler, then get a modified version with the replace method.

  • The non-limited API files cellobject.h, classobject.h, code.h, context.h, funcobject.h, genobject.h and longintrepr.h have been moved to the Include/cpython directory. Moreover, the eval.h header file was removed. These files must not be included directly, as they are already included in Python.h: Include Files. If they have been included directly, consider including Python.h instead. (Contributed by Victor Stinner in bpo-35134.)

I guess a more recent version than Cython 0.29.21 is required for Python 3.11:

/* Generated by Cython 0.29.21 */

Cython 0.29.25 fixes most bugs related to Python 3.11 and could be sufficient.
Cython 0.29.26 fixes an additional bug related to Python 3.11, but it might not be required.

  • Unit tests and/or doctests in docstrings
  • tox -e py310 passes locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • tox -e docs passes locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Coveralls passes)

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 27, 2022

One last error after upgrading to Cython 0.29.28:

    numcodecs/_shuffle.c: In function ‘__Pyx_AddTraceback’:
    numcodecs/_shuffle.c:438:62: error: dereferencing pointer to incomplete type ‘PyFrameObject’ {aka ‘struct _frame’}
      438 |   #define __Pyx_PyFrame_SetLineNumber(frame, lineno)  (frame)->f_lineno = (lineno)
          |                                                              ^~

I believe this one is fixed by Cython 0.29.29 - but it's not available on my Ubuntu 22.04 machine as a DEB package.

@DimitriPapadopoulos
Copy link
Contributor Author

Tried to install Cython 0.29.29, but pip complains this version is yanked:

$ pip install --user --ignore-installed Cython==0.29.29
Collecting Cython==0.29.29
  Using cached Cython-0.29.29-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.manylinux_2_24_x86_64.whl (1.9 MB)
WARNING: The candidate selected for download or install is a yanked version: 'cython' candidate (version 0.29.29 at https://files.pythonhosted.org/packages/b7/16/ccc814e0a063d1a4d71f2402b865beae046053d1d66b65b9e3e75b4bde2f/Cython-0.29.29-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.manylinux_2_24_x86_64.whl (from https://pypi.org/simple/cython/) (requires-python:>=2.6, !=3.0.*, !=3.1.*, !=3.2.*))
Reason for being yanked: https://github.com/cython/cython/issues/4796
Installing collected packages: Cython
Successfully installed Cython-0.29.29
$ 

Let's upgrade to Cython 0.29.30:

$ pip install --user --ignore-installed Cython==0.29.30
Collecting Cython==0.29.30
  Downloading Cython-0.29.30-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.manylinux_2_24_x86_64.whl (1.9 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.9/1.9 MB 17.1 MB/s eta 0:00:00
Installing collected packages: Cython
Successfully installed Cython-0.29.30
$ 

Re-generated numcodecs/_shuffle.c again.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 27, 2022

Last CI issue occurs while building docs, looks like a zfpy problem:

Installing collected packages: zfpy, snowballstemmer, pytz, alabaster, sphinxcontrib-serializinghtml, sphinxcontrib-qthelp, sphinxcontrib-jsmath, sphinxcontrib-htmlhelp, sphinxcontrib-devhelp, sphinxcontrib-applehelp, Pygments, mock, MarkupSafe, imagesize, docutils, babel, Jinja2, sphinx, sphinx-issues, numpydoc
  Attempting uninstall: zfpy
    Found existing installation: zfpy 1.0.0
    Uninstalling zfpy-1.0.0:
      Successfully uninstalled zfpy-1.0.0
  DEPRECATION: zfpy is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https://github.com/pypa/pip/issues/8559
  Running setup.py install for zfpy: started
  Running setup.py install for zfpy: finished with status 'error'
  error: subprocess-exited-with-error
  
  × Running setup.py install for zfpy did not run successfully.
  │ exit code: 1
  ╰─> [15 lines of output]
      running install
      /opt/hostedtoolcache/Python/3.11.0/x64/lib/python3.11/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
        warnings.warn(
      running build
      running build_ext
      building 'zfpy' extension
      creating build
      creating build/temp.linux-x86_64-cpython-311
      creating build/temp.linux-x86_64-cpython-311/build
      creating build/temp.linux-x86_64-cpython-311/build/python
      gcc -pthread -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -Iinclude -I/opt/hostedtoolcache/Python/3.11.0/x64/lib/python3.11/site-packages/numpy/core/include -I/opt/hostedtoolcache/Python/3.11.0/x64/include/python3.11 -c build/python/zfpy.c -o build/temp.linux-x86_64-cpython-311/build/python/zfpy.o
      gcc: error: build/python/zfpy.c: No such file or directory
      gcc: fatal error: no input files
      compilation terminated.
      error: command '/usr/bin/gcc' failed with exit code 1

It looks like no pre-built wheel is found, we attempt to build from source, but the build process is broken: LLNL/zfp#190.

Python 3.11 wheels have been added yesterday, perhaps we need to wait for them to build: LLNL/zfpy-wheels@ae6ca88

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review October 27, 2022 13:44
@jakirkham
Copy link
Member

Thanks Dimitri! 🙏

Could we actually drop the generated C files? There's some related discussion in issue ( #367 ) and issue ( #364 ). Maybe this helps simplify building for new Python versions going forward

@DimitriPapadopoulos
Copy link
Contributor Author

I hope I am not misunderstanding: yes, it should be possible to re-generate C files on the fly instead of pre-generating vendored C files.

However, do we need to do this in this specific PR? Perhaps when pyproject.toml support is added? I don't have much experience with Cython, and my initial goal in this PR was to support Python 3.11 with minimal changes.

@jakirkham
Copy link
Member

Sorry am just suggesting we delete the C files (generation on the fly should already happen without other changes)

Yeah not suggesting we make the pyproject.toml move. Agree that's tangential. Only referencing it in the context of dropping C files (since it was discussed in that issue as well)

@DimitriPapadopoulos
Copy link
Contributor Author

I see. Let me start by removing numcodecs/_shuffle.c first, checking if it is generated when building.

@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2022

This pull request introduces 1 alert when merging 4112359 into a3e05fd - view on LGTM.com

new alerts:

  • 1 for Syntax error

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 27, 2022

I had to add a pyproject.toml file to define build-time dependencies and make sure Cython is installed before setup() is called. With Cython now always available, I am able t remove all vendored C files.They will be regenerated on the fly by Cython.

With vendored C files out of the way, moving to Python 3.11 is pretty easy. No need to re-generate C files with a version of Cython compatible with Python 3.11, because they will re-generated on the fly.

@jakirkham
Copy link
Member

Awesome thank you for digging in here Dimitri! 🙏

Recognize we have some code debt here in the build setup. Hoping we can chip away at it to simplify things both now and in the future 😉

@jakirkham
Copy link
Member

FWIW here's the cpuinfo module referred to on CI. This actually comes from the py-cpuinfo library. If we can't get setuptools to recognize it, perhaps we should devendor it and py-cpuinfo to our build requirements (maybe a good idea in any case)

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 28, 2022

I hadn't noticed this cpuinfoissue until now. It must have been triggered by one of my very recent changes. Will look into it.

@jakirkham
Copy link
Member

No worries. Looks like that fixed it.

The Python 3.11 failure is happening because zfpy hasn't built wheels for Python 3.11 yet. Would suggest we just adjust requirements_dev.txt to only install zfpy for Python versions before 3.11.

Also sorry for not commenting on the diff (it is below the fold)

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 28, 2022

The cpuinfo issue was caused by this line I had tried to add:

build-backend = "setuptools.build_meta"

I think this line will be added with setup.pypyproject.toml, but for now we should follow Fallback Behaviour:

If a project does not have a pyproject.toml file containing a build-system section, it will be assumed to have the following backend settings:

[build-system]
requires = ["setuptools>=40.8.0", "wheel"]
build-backend = "setuptools.build_meta:__legacy__"

If a project has a build-system section but no build-backend, then:

  • It is expected to include setuptools and wheel as build requirements. An error is reported if the available version of setuptools is not recent enough.
  • The setuptools.build_meta:__legacy__ build backend will be used.

@DimitriPapadopoulos
Copy link
Contributor Author

We could indeed devendor py-cpuinfo, but I suggest we keep that for another PR.

@jakirkham
Copy link
Member

Yep was reading about this as well. There's probably some things we need to move from MANIFEST.in to pyproject.toml. That said, agree we can think about that in another PR :)

@DimitriPapadopoulos
Copy link
Contributor Author

Is zfpy optional in requirements_dev.txt?

It is also found in requirements_rtfd.txt by the way. I cannot understand whether the lack of Python 3.11 wheels might impact the docs build.

@jakirkham
Copy link
Member

It's a fair question. Yes it is optionally used here. If it is not present, that codec isn't enabled.

Think RTD should be ok since we don't need to move that to Python 3.11 (it can stay on whatever Python version it is now).

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 28, 2022

It looks like docs are built for all versions of Python, using requirements_rtfd.txt, at least on Linux:

- name: Build Docs
run: |
pip install -r requirements_rtfd.txt
cd docs
sphinx-build -W -b html -d {envtmpdir}/doctrees . {envtmpdir}/html

Hence the Python 3.11 failure.

Perhaps we need a different job for docs, using a single version of Python.

@jakirkham
Copy link
Member

jakirkham commented Oct 28, 2022

Good point.

Maybe that is a holdover from before we had RTD running on PRs? It might make sense to drop the doc step. Though that's probably a bigger change than we want to pursue in this PR.

Perhaps another conditional in that requirement file would be sufficient?

@mbargull
Copy link

Might want to exclude them from MANIFEST.in to avoid regressions.


FYI, at some point in the future when Cython 3.x is available, these things should "sort themselves out" due to these changes:
https://github.com/cython/cython/blob/3.0.0a11/CHANGES.rst

[...]

  • cythonize() and the corresponding CLI command now regenerate the output files
    also when they already exist but were generated by a different Cython version.

[...]

  • The environment variable CYTHON_FORCE_REGEN=1 can be used to force cythonize
    to regenerate the output files regardless of modification times and changes.

[...]

File pyproject.toml is required to install Cython before setup():
	https://setuptools.pypa.io/en/latest/build_meta.html

Vendored C files have been deleted. They will re-generated by Cython
on the fly.

For developers who want to generate C files using Cython locally,
we enforce Cython>=0.29.30. That is the minimal version required
to generate a _shuffle.c file compatible with Python 3.11.
Avoid zpfy when building with Python 3.11 for now, to avoid CI errors.
@jakirkham jakirkham enabled auto-merge (squash) October 28, 2022 16:19
@jakirkham
Copy link
Member

Thanks Dimitri! 🙏

Will merge once CI passes

@jakirkham
Copy link
Member

Raised issue ( LLNL/zfp#192 ) about zfpy Python 3.11 wheels

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