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

MAINT: Start testing with Python 3.11. #21308

Merged
merged 2 commits into from
Jun 2, 2022
Merged

Conversation

felixxm
Copy link
Contributor

@felixxm felixxm commented Apr 7, 2022

We're having issues building numpy with Python 3.11 in Django tests, so I wanted to help make numpy compatible with Python 3.11.

@felixxm felixxm changed the title MAINT: Start testing with Python 3.10. MAINT: Start testing with Python 3.11. Apr 7, 2022
@mattip
Copy link
Member

mattip commented Apr 7, 2022

There are 3 warnings about ob_shash. One is from

((PyBytesObject *)obj)->ob_shash = -1;

The others come from cython.

numpy/core/src/multiarray/scalarapi.c:709:13: warning: ‘ob_shash’ is deprecated [-Wdeprecated-declarations]
numpy/random/_mt19937.c:7144:13: warning: ‘ob_shash’ is deprecated [-Wdeprecated-declarations]
numpy/random/_mt19937.c:7145:13: warning: ‘ob_shash’ is deprecated [-Wdeprecated-declarations]

@felixxm
Copy link
Contributor Author

felixxm commented Apr 7, 2022

There are 3 warnings about ob_shash. One is from

((PyBytesObject *)obj)->ob_shash = -1;

This looks unnecessary 🤔, cached hash value for obj should not be computed yet.

@mattip
Copy link
Member

mattip commented Apr 7, 2022

Thanks for getting this going. I think we should throw in a if version_info.releaselevel == alpha guard around the warnings check for now so we can produce a package that can be tested by other downstream consumers on the next cron build of NumPy.

Then this will start failing on the beta releases if we have not cleared up the warnings. Does that make sense?

@mattip
Copy link
Member

mattip commented Apr 7, 2022

... assuming tests pass after we can get a build

@mattip
Copy link
Member

mattip commented Apr 7, 2022

Hmm. Now the compile failed with another cython error, this one has been fixed and will be part of 0.29.29.

numpy/random/_mt19937.c:438:62: error: dereferencing pointer to incomplete type ‘PyFrameObject’ {aka ‘struct _frame’}
  438 |   #define __Pyx_PyFrame_SetLineNumber(frame, lineno)  (frame)->f_lineno = (lineno)
      |                                                              ^~
numpy/random/_mt19937.c:7855:5: note: in expansion of macro ‘__Pyx_PyFrame_SetLineNumber’
 7855 |     __Pyx_PyFrame_SetLineNumber(py_frame, py_line);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~

I wonder if there is an easy way to get CI to use Cython HEAD from a git commit for the 3.11 run?

@felixxm felixxm force-pushed the python-3.11-tests branch 2 times, most recently from 7a62798 to ac0838e Compare April 7, 2022 10:35
@felixxm
Copy link
Contributor Author

felixxm commented Apr 7, 2022

Amazing, only one failure on Python 3.11 👍

@felixxm felixxm force-pushed the python-3.11-tests branch 2 times, most recently from 6a5fa8a to 08569fc Compare April 7, 2022 11:10
@mattip
Copy link
Member

mattip commented Apr 7, 2022

I could track the ob_shash = 1 code back to 16 years ago. Do we need to set the hash to -1 when we are sharing the data between the python object and the scalar?

@mattip
Copy link
Member

mattip commented Apr 7, 2022

Looks like it is passing. Could you open an issue for updating to Cython 0.29.29, working around the ob_shash assignment, and reverting the alpha warning removal so we don't forget to update/fix/revert them?

@charris
Copy link
Member

charris commented Apr 10, 2022

I'd prefer to wait for a Cython release rather than complicate things. There are also Cython versions specified in pyproject.toml and setup.py. Looks like setup.py needs an update in any case. We normally start testing against Python with the beta releases. NumPy 1.23.x will be the first release to support 3.11. Note that we will be branching 1.22, in the middle of May, so the start of 1.23 development is about five weeks out.

@h-vetinari
Copy link
Contributor

NumPy 1.23.x will be the first release to support 3.11. Note that we will be branching 1.22, in the middle of May, so the start of 1.23 development is about five weeks out.

Did you mean 1.24.x (because 1.22 has been released so the branching in May is going to be for 1.23...)?

If so, I'd like to ask that if the backports aren't too onerous, it would be nice to start supporting python 3.11 already with a 1.23 point-release (similar to how 1.19.3 added support for python 3.9 and 1.21.3 added support for 3.10)

@charris
Copy link
Member

charris commented Apr 15, 2022

it would be nice to start supporting python 3.11 already with a 1.23 point-release

That is how we will do it once the first release candidate is out. We will start testing with the first beta, due 2022-05-06. And yes, it is 1.23 that will be branched next month.

@felixxm felixxm force-pushed the python-3.11-tests branch from 08569fc to de457df Compare May 10, 2022 15:37
@felixxm
Copy link
Contributor Author

felixxm commented May 10, 2022

Rebased.

test_requirements.txt Outdated Show resolved Hide resolved
@felixxm felixxm force-pushed the python-3.11-tests branch from de457df to ee84d50 Compare May 17, 2022 13:28
@felixxm
Copy link
Contributor Author

felixxm commented May 17, 2022

@mattip I rebased from the main branch. Don't you think it's worth keeping the last commit? 🤔 It allows installing cython without grepping the requirements file.

@felixxm felixxm force-pushed the python-3.11-tests branch from ee84d50 to 88197b6 Compare May 17, 2022 13:33
@EwoutH
Copy link
Contributor

EwoutH commented May 23, 2022

Now that #21543 is merged, could you rebase this PR? The Python 3.11 CI now might pass without any errors!

@felixxm felixxm force-pushed the python-3.11-tests branch from 88197b6 to 34b9ac6 Compare May 23, 2022 07:24
@felixxm
Copy link
Contributor Author

felixxm commented May 23, 2022

Now that #21543 is merged, could you rebase this PR? The Python 3.11 CI now might pass without any errors!

Thanks 👍 , rebased.

@EwoutH
Copy link
Contributor

EwoutH commented May 23, 2022

The previous error is now gone, but it seems that test_generic_alias.py still doesn't pass in the CI with Python 3.11, generating the following failures:

Failure summary
=================================== FAILURES ===================================
_________________ TestGenericAlias.test_pass[__dir__-<lambda>] _________________

self = <numpy.typing.tests.test_generic_alias.TestGenericAlias object at 0x7f7475f1a810>
name = '__dir__', func = <function TestGenericAlias.<lambda> at 0x7f74850974c0>

    @pytest.mark.parametrize("name,func", [
        ("__init__", lambda n: n),
        ("__init__", lambda n: _GenericAlias(np.ndarray, Any)),
        ("__init__", lambda n: _GenericAlias(np.ndarray, (Any,))),
        ("__init__", lambda n: _GenericAlias(np.ndarray, (Any, Any))),
        ("__init__", lambda n: _GenericAlias(np.ndarray, T1)),
        ("__init__", lambda n: _GenericAlias(np.ndarray, (T1,))),
        ("__init__", lambda n: _GenericAlias(np.ndarray, (T1, T2))),
        ("__origin__", lambda n: n.__origin__),
        ("__args__", lambda n: n.__args__),
        ("__parameters__", lambda n: n.__parameters__),
        ("__reduce__", lambda n: n.__reduce__()[1:]),
        ("__reduce_ex__", lambda n: n.__reduce_ex__(1)[1:]),
        ("__mro_entries__", lambda n: n.__mro_entries__([object])),
        ("__hash__", lambda n: hash(n)),
        ("__repr__", lambda n: repr(n)),
        ("__getitem__", lambda n: n[np.float64]),
        ("__getitem__", lambda n: n[ScalarType][np.float64]),
        ("__getitem__", lambda n: n[Union[np.int64, ScalarType]][np.float64]),
        ("__getitem__", lambda n: n[Union[T1, T2]][np.float32, np.float64]),
        ("__eq__", lambda n: n == n),
        ("__ne__", lambda n: n != np.ndarray),
        ("__dir__", lambda n: dir(n)),
        ("__call__", lambda n: n((1,), np.int64, BUFFER)),
        ("__call__", lambda n: n(shape=(1,), dtype=np.int64, buffer=BUFFER)),
        ("subclassing", lambda n: _get_subclass_mro(n)),
        ("pickle", lambda n: n == pickle.loads(pickle.dumps(n))),
    ])
    def test_pass(self, name: str, func: FuncType) -> None:
        """Compare `types.GenericAlias` with its numpy-based backport.
    
        Checker whether ``func`` runs as intended and that both `GenericAlias`
        and `_GenericAlias` return the same result.
    
        """
        value = func(NDArray)
    
        if sys.version_info >= (3, 9):
            value_ref = func(NDArray_ref)
>           assert value == value_ref
E           AssertionError: assert ['T', '__abs_...array__', ...] == ['T', '__abs_...array__', ...]
E             At index 99 diff: '__xor__' != '__unpacked__'
E             Right contains one more item: 'view'
E             Use -v to get the full diff

func       = <function TestGenericAlias.<lambda> at 0x7f74850974c0>
name       = '__dir__'
self       = <numpy.typing.tests.test_generic_alias.TestGenericAlias object at 0x7f7475f1a810>
value      = ['T', '__abs__', '__add__', '__and__', '__args__', '__array__', ...]
value_ref  = ['T', '__abs__', '__add__', '__and__', '__args__', '__array__', ...]

../../builds/venv/lib/python3.11/site-packages/numpy-1.23.0.dev0+1280.ga2d2b5691-py3.11-linux-x86_64.egg/numpy/typing/tests/test_generic_alias.py:86: AssertionError

@h-vetinari
Copy link
Contributor

It's perhaps not surprising that there's new typing attributes in python 3.11:

>           assert value == value_ref
E           AssertionError: assert ['T', '__abs_...array__', ...] == ['T', '__abs_...array__', ...]
E             At index 99 diff: '__xor__' != '__unpacked__'
E             Right contains one more item: 'view'

The test already indicates the dependence on the cpython version (if sys.version_info >= (3, 9):), so it seems this just needs an additional if sys.version_info >= (3, 11): branch (and probably some work on the numpy typing backport)

@BvB93
Copy link
Member

BvB93 commented May 23, 2022

I would suggest putting in a sys.version_info guard on the relevant test until the first release candidate: (3, 11, 0, "candidate").

The relevant types.GenericAlias class is still quite actively being updated right now (e.g. python/cpython#92335), in addition to it not being 100% clear to me yet which (if any) parts should be backported to npt._GenericAlias.

@mattip
Copy link
Member

mattip commented May 27, 2022

#21605 is merged but rerunning the failed CI job did not clear the error. Maybe a rebase on main is needed?

@EwoutH
Copy link
Contributor

EwoutH commented May 30, 2022

@felixxm Could you rebase this PR?

@felixxm felixxm force-pushed the python-3.11-tests branch from 34b9ac6 to 3aff21e Compare May 30, 2022 11:36
@felixxm
Copy link
Contributor Author

felixxm commented May 30, 2022

Rebased

@felixxm
Copy link
Contributor Author

felixxm commented Jun 2, 2022

It works 🚀 🎉

@charris
Copy link
Member

charris commented Jun 2, 2022

Could you get rid of cython_test_requirements. It isn't needed, the requirements are handled in the usual files.

@felixxm felixxm force-pushed the python-3.11-tests branch from 1d02d1b to f04688d Compare June 2, 2022 17:12
@felixxm
Copy link
Contributor Author

felixxm commented Jun 2, 2022

Could you get rid of cython_test_requirements. It isn't needed, the requirements are handled in the usual files.

Sure if you think it's not worth changing. Removed.

@charris charris merged commit af8d52e into numpy:main Jun 2, 2022
@charris
Copy link
Member

charris commented Jun 2, 2022

Thanks @felixxm .

@EwoutH
Copy link
Contributor

EwoutH commented Jun 2, 2022

Could this PR also be backported to the 1.23 maintenance branch?

@charris
Copy link
Member

charris commented Jun 2, 2022

@EwoutH It is already marked for backport.

@charris charris removed this from the 1.23.1 release milestone Jun 2, 2022
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 2, 2022
@felixxm felixxm deleted the python-3.11-tests branch June 3, 2022 04:00
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.

8 participants