-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Update qiskit.utils.wrap_method
for Python 3.11
#9310
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Nice this seems to work! The 3.11 tests are failing due to NumPy, but that should be fixed now. |
Pull Request Test Coverage Report for Build 3878635934
💛 - Coveralls |
qiskit.utils.wrap_method
for Python 3.11.1
qiskit.utils.wrap_method
for Python 3.11.1qiskit.utils.wrap_method
for Python 3.11
Co-authored-by: Julien Gacon <[email protected]>
While this lets the tests run, I think it is not quite right in general. |
Now I think the change is correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, I didn't know about getattr_static
but that looks like exactly what we were trying to hack together before (and what was causing issues for us on 3.11.1). I'm going to hold off on automerge until @jakelishman can take a look though since he wrote all of this decorator code (and I've lost most of the context I previously had for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks Will. I think I didn't know about getattr_static
when I wrote that first implementation, which is part of why it got super hacky.
* Revert "[Test] Pin maximum python version in CI to <3.11.1 (#9296)" This reverts commit 07e0a2f. * Do not treat __init_subclass__ as a special type method * Release note * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Use inspect.getattr_static to bypass descriptor call * Update release note * Update wrap_method test * Adjust wording on release note Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 0344a1c)
* Revert "[Test] Pin maximum python version in CI to <3.11.1 (Qiskit#9296)" This reverts commit 07e0a2f. * Do not treat __init_subclass__ as a special type method * Release note * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Use inspect.getattr_static to bypass descriptor call * Update release note * Update wrap_method test * Adjust wording on release note Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Revert "[Test] Pin maximum python version in CI to <3.11.1 (Qiskit#9296)" This reverts commit 07e0a2f. * Do not treat __init_subclass__ as a special type method * Release note * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Use inspect.getattr_static to bypass descriptor call * Update release note * Update wrap_method test * Adjust wording on release note Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Revert "[Test] Pin maximum python version in CI to <3.11.1 (Qiskit#9296)" This reverts commit 07e0a2f. * Do not treat __init_subclass__ as a special type method * Release note * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Use inspect.getattr_static to bypass descriptor call * Update release note * Update wrap_method test * Adjust wording on release note Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 0344a1c)
* Fix NumPy 1.24.0 compatibility and pin `coverage<7.0` (#9305) * fix Kraus from (array, None) * fix triu_to_dense test * fix instruction comparison * skip snobfit if numpy 1.24.0 or above is installed Co-authored-by: ElePT <[email protected]> * pin coverage <7.0 * add links to Kraus and snobfit issues * retrigger CI Co-authored-by: ElePT <[email protected]> (cherry picked from commit 9733fc0) * Update `qiskit.utils.wrap_method` for Python 3.11 (#9310) * Revert "[Test] Pin maximum python version in CI to <3.11.1 (#9296)" This reverts commit 07e0a2f. * Do not treat __init_subclass__ as a special type method * Release note * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Use inspect.getattr_static to bypass descriptor call * Update release note * Update wrap_method test * Adjust wording on release note Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 0344a1c) * Relax constraints on jupyter-core and ipywidgets (#9364) These were originally added in #9105 and #9272 respectively, but the original problem package `seaborn` has released since then, which may have fixed things. This removes some now-unnecessary suppressions from image-related packages, and adds the new suppression for the pyzmq problem, which is Jupyter's domain to handle. The extra environment variable in the images test run is to eagerly move to new default behaviour starting in jupyter-core 6; there is no need for us to pin the package too low, since this warning is just encouraging people to proactively test the new behaviour, and it doesn't cause our suite problems. * Refactor coverage CI workflow (#9361) This relaxes the constraint on `coverage` added in #9305. The issue there is actually the now unmaintained `coveragepy-lcov` package is not compatible with Coverage.py 7.0. However, we only needed `coveragepy-lcov` to convert Coverage's format into LCOV, which is a feature Coverage has had itself since version 6.0. This commit also updates some parts of the coverage workflow that were old: - there are new versions of the Actions `checkout` and `setup-python`, which swap to using Node 16 rather than Node 12, which is deprecated in GHA - `grcov` is packaged and installable from `cargo` now, rather than needing a manual hard-coded pull from GitHub - we can have `grcov` only keep the parts we care about immediately, rather than converting everything and later discarding it Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 7955d92) Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: Will Shanks <[email protected]>
* Revert "[Test] Pin maximum python version in CI to <3.11.1 (Qiskit/qiskit#9296)" This reverts commit 07e0a2fc79bada7c1fbf0594f4ad33934f70b7e2. * Do not treat __init_subclass__ as a special type method * Release note * Apply suggestions from code review Co-authored-by: Julien Gacon <[email protected]> * Use inspect.getattr_static to bypass descriptor call * Update release note * Update wrap_method test * Adjust wording on release note Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
Fixed the behavior of
qiskit.utils.wrap_method
when applied to aclassmethod
that is ontype
. Code changes in Python 3.11 and 3.11.1 revealed thatwrap_method
did not avoid calling aclassmethod
desscriptor's__get__
method.wrap_method
was changed to useinspect.getattr_static
to avoid calling__get__
.Details and comments
The poor handling of
classmethod
bywrap_method
was first noticed with Python 3.11.1 as described in #9291.qiskit.test.decorators.enforce_subclasses_call
wrapsQiskitBaseTestCase.__init_subclass__
usingwrap_method
. Prior to 3.11.1, this meant wrappingobject.__init_subclass__
(which is a builtin method that behaves differently), but in 3.11.1unittest.TestCase
added an__init_subclass__
method. This change meant that nowwrap_method
was wrapping a "real"classmethod
(__init_subclass__
is a special case but it behaves enough like aclassmethod
for this issue).In Python 3.11.0, the behavior of the bound method descriptor changed (specifically, it was this PR and the change to
funcobject.c
there). The following code illustrates the difference in behavior:In Python 3.10, this gives
while in Python 3.11 it gives
So in Python 3.10 and earlier, the bound method's descriptor returns itself. With Python 3.11 though, the descriptor returns the unbound function.
Prior to the change here,
wrap_method
usedobject.__getattribute__
andtype.__getattribute__
to retrieve the descriptor for the method being wrapped so that its__get__
could be called by the wrapping descriptor. For methods that are not indir(type)
, it usedobject.__getattribute__(cls, name)
to retrieve the descriptor in a loop over everycls
in the wrapped class's MRO passing onAttributeError
.object.__getattribute__(cls, name)
checkstype(cls)
(sotype
) for a descriptor and otherwise checkscls.__dict__
forname
, raising anAttributeError
ifname
is not found (the code is here). By doing this loop over the MRO,name
is eventually found. Note, however, thatobject.__getattribute__
follows a different path ifname
is a descriptor intype(cls)
(so intype
). To avoid this different path,wrap_method
checkedname
againstdir(type)
and usedtype.__getattribute__
instead.type.__getattribute__(cls, name)
loops over the MRO ofcls
looking forname
and when it finds it calls.__get__(None, cls)
on it (the code is here). This call returns a bound method version of the classmethod withcls
bound and this is where the Python 3.11 change in behavior comes in. Inqiskit.utils.classtools._WrappedMethod
,__get__
tries to delegate to the wrapped method by calling.__get__
on it but for thistype
case that means calling.__get__
on a bound method instead of theclassmethod
and so for 3.11 getting back an unbound function that expects an extra first argument containing thecls
.So in Python 3.11.1 with the addition of
__init_subclass__
tounittest.TestCase
, thewrap_method
call around__init_subclass__
onQiskitBaseTestCase
meant that the definition ofQiskitTestCase(QiskitBaseTestCase)
now tried to call the wrapped__init_subclass__
as an unbound function without passing it the requiredcls
argument and this led to the error. In 3.11.0, the handling of the descriptor did not matter becauseobject.__init_subclass__
was the method being wrapped and it has a*args, **kwargs
signature and does nothing, so it did not matter that it was invoked incorrectly (see here).Closes #9291