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

Fix EventedModel signatures with PySide2 imported #2265

Merged
merged 2 commits into from Feb 14, 2021

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Feb 13, 2021

Description

fixes #2264. This fixes a strange and unfortunate clash resulting from decisions made by PySide2 and pydantic (neither of which I feel we'd have much luck changing upstream). I put a more thorough explanation in the code docstring itself, but here's the gist:

  1. when PySide2 fixed this bug, they made the decision to make (all?) python __signature__ objects dynamically writable (as they should be), but only once (?!).
  2. Pydantic, in attempting to fix signature conflicts in cases where a class defines a __call__ method, uses a descriptor object called ClassAttribute to wrap their custom inspect.Signature object during class creation. such that cls.__signature__ = ClassAttribute('__signature__', actual_signature)
  3. when accessing obj.__signature__ PySide2 seems to just return that ClassAttribute descriptor rather than calling its __get__ method... so inspect.signature fails.

This PR temporarily monkeypatches pydantic's ClassAttribute object to just pass through the signature object directly when constructing our classes... then undoes the patch.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

How has this been tested?

  • added a test to make sure that signatures are working
  • all tests pass with my change

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Feb 13, 2021

Codecov Report

Merging #2265 (69a47a2) into master (dcfdb0c) will decrease coverage by 0.00%.
The diff coverage is 62.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
- Coverage   67.10%   67.09%   -0.01%     
==========================================
  Files         400      400              
  Lines       33652    33672      +20     
==========================================
+ Hits        22581    22593      +12     
- Misses      11071    11079       +8     
Impacted Files Coverage Δ
napari/utils/events/evented_model.py 50.00% <44.44%> (+0.79%) ⬆️
napari/utils/events/_tests/test_evented_model.py 100.00% <100.00%> (ø)
napari/utils/events/event.py 49.56% <0.00%> (-0.88%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcfdb0c...69a47a2. Read the comment docs.

Copy link
Contributor

@sofroniewn sofroniewn left a comment

Choose a reason for hiding this comment

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

LGTM! Great debugging and crazy situation!!

def no_class_attributes():
"""Context in which pydantic.main.ClassAttribute just passes value 2.

Due to a very annoying decision by PySide2, all class ``__signature__``
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to the issue where you raised #2264, maybe even this PR #2265 too, I can just imagine someone who reads this wanting to find there way back here too

@sofroniewn sofroniewn added this to the 0.4.6 milestone Feb 13, 2021
@sofroniewn sofroniewn added the bug Something isn't working label Feb 13, 2021
@tlambert03 tlambert03 merged commit 46f242b into napari:master Feb 14, 2021
@tlambert03 tlambert03 deleted the fix-pyside2-signature branch February 14, 2021 13:14
@basnijholt
Copy link

@tlambert03 did you ever raise this issue with PySide2?

@tlambert03
Copy link
Member Author

Hi @basnijholt, no I didn't create a new issue. I did find this issue: https://bugreports.qt.io/browse/PYSIDE-1004 ... so they're aware of it to some degree. But I haven't checked to see how more recent versions of PySide2/6 are handling this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PySide2 conflicting with pydantic signatures
3 participants