Skip to content

Set __hash__ to None for types that defines __eq__ but not __hash__ #2291

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 26, 2020
Merged

Set __hash__ to None for types that defines __eq__ but not __hash__ #2291

merged 1 commit into from
Jul 26, 2020

Conversation

sizmailov
Copy link
Contributor

PR patches py::class_<>::def as discussed in #2287

fixes #2191

sizmailov
sizmailov commented Jul 11, 2020
View reviewed changes
YannickJadoul
YannickJadoul approved these changes Jul 11, 2020
View reviewed changes
@YannickJadoul
Copy link
Collaborator

Oh, what about docs, cfr. #2287? Do we need to mention this now or is it just "fixed". Or are we merging that as reworked #2278?

@bstaletic
Copy link
Collaborator

Oh, what about docs, cfr. #2287? Do we need to mention this now or is it just "fixed". Or are we merging that as reworked #2278?

Unless I'm missing something, we can just treat #2191 as resolved and leave the docs alone.

@YannickJadoul
Copy link
Collaborator

Unless I'm missing something, we can just treat #2191 as resolved and leave the docs alone.

Fine with me. I just saw @jbarlow83's remark (#2287 (comment)).
@ixje, you brought up this issue in the first place; do you think further docs are needed, or is just being consistent with Python good enough?

rwgk
rwgk approved these changes Jul 11, 2020
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just a couple minor questions about a comment and a test.

@ixje
Copy link

ixje commented Jul 11, 2020

@ixje, you brought up this issue in the first place; do you think further docs are needed, or is just being consistent with Python good enough?

I did the docs PR when we still had the believe that the impact of actually solving it was too high. I'll close that one in favour of this, as it is no longer required. Thanks everybody for resolving this ❤️

@ixje ixje mentioned this pull request Jul 11, 2020
Closed
@YannickJadoul
Copy link
Collaborator

I think this was approved by everyone? I'm merging.

Thank you very much, once again, @sizmailov! :-)

@YannickJadoul YannickJadoul merged commit 7b067cc into pybind:master Jul 26, 2020
@henryiii henryiii added the bug label Aug 19, 2020
tensorflow-copybara pushed a commit to tensorflow/tensorflow that referenced this pull request Aug 21, 2020
@tensorflower-gardener
Adding __hash__ for forward compatibility with pybind11 update.
2f29946 
Backward compatible.

Relevant pybind11 change: pybind/pybind11#2291
Note: This pybind11 change makes pybind11 compatible with native Python behavior:
* https://docs.python.org/3/reference/datamodel.html#object.__hash__
* A class that overrides __eq__() and does not define __hash__() will have its __hash__() implicitly set to None.
PiperOrigin-RevId: 327879230
Change-Id: I8573b761ec94e8889b4eb5ac30a71ee0314e6578
@sizmailov sizmailov mentioned this pull request Sep 8, 2020
Closed
@Skylion007 Skylion007 mentioned this pull request Jan 7, 2022
Closed
3 tasks
spham-amzn added a commit to aws-lumberyard-dev/o3de that referenced this pull request Jul 19, 2022
@spham-amzn
Fix error from PythonProxyObject with PyBind11 update to 2.7.1 that r…
c154561 
…esults in an 'unhashable type' for PythonProxyObject.

(See pybind/pybind11#2291 )

Signed-off-by: Steve Pham <[email protected]>
spham-amzn added a commit to o3de/o3de that referenced this pull request Aug 22, 2022
@spham-amzn
Python 3.10 Update (#11216)
607c548 
* Update pyside2 package and deprecated message
* Update pytest and pybind versions
* Fix python 3.10 where subprocess.run is outputing its --help argument result from calling SerializeContextTools to stderr instead of stdout, even though the result code is zero
* Fix error from PythonProxyObject with PyBind11 update to 2.7.1 that results in an 'unhashable type' for PythonProxyObject.
(See pybind/pybind11#2291 )
* Fix deprecations errors caused be direct construction of EditorTestClass
* Fix crash caused by calling trying to access 'PyErr_Occurred' after pybind11::finalize_interpreter.
"Shut down the Python interpreter. No pybind11 or CPython API functions can be called after this. In addition, pybind11 objects must not outlive the interpreter:"
* Fix crash caused by calling trying to access 'PyErr_Occurred' after pybind11::finalize_interpreter.
    According to  https://pybind11.readthedocs.io/en/stable/reference.html :
    "Shut down the Python interpreter. No pybind11 or CPython API functions can be called after this. In addition, pybind11 objects must not outlive the interpreter:"
* Fix unit test that expected an unknown result from a crash, but is actually receiving a fail result
* Updates to support Python 3.10 on Linux
* Update versions in cmake scripts to 3.10.5
* Update 3rd Party Package References to python 3.10.5 related dependencies (pybind11, pyside2)
* Update PythonLoader_Linux to support new version
* Add PythonLoader instance to PythonSystemComponent to load/unload python3.10.5.so properly to fix new 3.8+ issue with undefined symbols (see https://docs.python.org/3/whatsnew/3.8.html#changes-in-the-c-api)
* Fix python 3.10 syntax error (using 'is' instead of '==' for an integer comparison)
* Minor hack for 'test_single_crash_test' to resolve an assertion based on the expected result type
* Update linux package hash for OpenImageIO / OpenColorIO
* Add Pyside2::Tools dependency for QtForPython
* Replace hard coded python shared library name with one that is extracted from the python library target
* Cleanup norecursedirs in pytest.ini
* Fix mac compile error :

Users/spham/github/o3de/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceDomGenerator.cpp:53:44: error: no viable conversion from 'optional<reference_wrapper<AzToolsFramework::Prefab::Instance>>' to 'optional<reference_wrapper<const AzToolsFramework::Prefab::Instance>>'
            InstanceOptionalConstReference focusedInstance = prefabFocusInterface->GetFocusedPrefabInstance(s_editorEntityContextId);
                                           ^
* Replace hardcoded python major/minor version number strings with a define passed in based on LY_PYTHON_VERSION_MAJOR_MINOR defined in LYPython.cmake
* Replace StringFunc::Path with AZ::IO::FixedPath to retrieve the filename from a path
* Applying suggestion from PR comments on reporting the state of a stoppython call

Signed-off-by: Steve Pham <[email protected]>
@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk approved these changes

@bstaletic bstaletic bstaletic left review comments

@YannickJadoul YannickJadoul YannickJadoul approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overriding __eq__ does not set __hash__ to None
6 participants
@sizmailov @YannickJadoul @bstaletic @ixje @rwgk @henryiii