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.10 compatiblity fixes #2856

Merged
merged 3 commits into from May 8, 2024

Conversation

k-dominik
Copy link
Contributor

@k-dominik k-dominik commented May 6, 2024

One of our test configurations picked up Python 3.10 (which is fine for the "lirbary" usage). This resulted in some minor errors:

  1. Threadings condition.notifyAll has been marked for deprecation in favor of condition.notify_all. In TestCompatibilityChecks.test_access_opaque_slot_value_should_not_warn_or_raise we check that no warning is raised. This was of course broken by the deprecation. Rather than adapting the test I preferred keeping up with Python
  2. Python stopped implicitly converting types in function calls -> results in errors/segfaults in pyqt. Explicitly converted types or made sure e.g. integer value division is used (e.g. in draw methods of featureTableWidget).

fixes #2852

@k-dominik k-dominik force-pushed the dbg-osx-test-failure-on-ci branch from e437f0a to afe12e6 Compare May 6, 2024 13:51
Copy link

codecov bot commented May 6, 2024

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 55.96%. Comparing base (1df74b8) to head (a51c4c1).

Files Patch % Lines
ilastik/widgets/featureTableWidget.py 60.00% 2 Missing ⚠️
ilastik/widgets/preView.py 66.66% 2 Missing ⚠️
...stik/applets/counting/countingGuiBoxesInterface.py 0.00% 1 Missing ⚠️
lazyflow/operator.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
+ Coverage   55.95%   55.96%   +0.01%     
==========================================
  Files         533      533              
  Lines       62300    62293       -7     
  Branches     8549     8548       -1     
==========================================
+ Hits        34859    34865       +6     
+ Misses      25684    25675       -9     
+ Partials     1757     1753       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@k-dominik k-dominik changed the title debugging ci Python 3.10 compatiblity fixes May 6, 2024
@k-dominik k-dominik force-pushed the dbg-osx-test-failure-on-ci branch from 8f0ff31 to 9e5ed6d Compare May 6, 2024 15:59
@k-dominik k-dominik marked this pull request as ready for review May 6, 2024 18:44
@k-dominik k-dominik requested a review from btbest May 7, 2024 08:17
Copy link
Contributor

@btbest btbest left a comment

Choose a reason for hiding this comment

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

Nicely tracked down 🫡 This means these bits of code actually weren't python3.10 compatible this whole time..?

There are three drawEllipse calls in the codebase that use old_div and could hence pass floats to the Qt function (which I assume now only works with int); featureTableWidget.py:174 and two in preView.py.

I suppose to be absolutely sure, one would have to explicitly int() the division results as well, since 6//2.0 is 2.0, but I think we'll be fine assuming that we're only passing ints into those divisions into the first place.

@k-dominik
Copy link
Contributor Author

k-dominik commented May 8, 2024

There are three drawEllipse calls in the codebase that use old_div and could hence pass floats to the Qt function (which I assume now only works with int); featureTableWidget.py:174 and two in preView.py.

ok - I will make those old_div -> // as well for better consistency, too, thank you for pointing to that.

The type annotations for pyqt help a lot with the int issue - so in most cases pyright tells me it's allright without casting. But yes, to be super sure we could cast (but I'd like to avoid this noise, as it's also sends the message that we don't trust the type annots)

@k-dominik k-dominik force-pushed the dbg-osx-test-failure-on-ci branch from 9e5ed6d to d4b72ab Compare May 8, 2024 09:19
k-dominik and others added 3 commits May 8, 2024 11:36
otherwise these might segfault, python stopped implicitly converting in
Python 3.10

Co-authored-by: Benedikt Best <63287233+btbest@users.noreply.github.com>
`notifyAll` would cause a deprecation warning that added a test failure
with Python 3.10.
There is no need to keep using the camel case alias.
this function would not work in any case, referencing attributes that
have long been removed from that class.
Also `adjustRectForImage` is never used.
@k-dominik k-dominik force-pushed the dbg-osx-test-failure-on-ci branch from d4b72ab to a51c4c1 Compare May 8, 2024 09:36
@k-dominik k-dominik merged commit dc5bf54 into ilastik:main May 8, 2024
16 checks passed
@k-dominik k-dominik deleted the dbg-osx-test-failure-on-ci branch May 8, 2024 11:01
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.

Test failure for gh actions on MacOS
2 participants