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

Reinstanting RmqThreadCommunicator tests #124

Merged
merged 4 commits into from
Oct 20, 2022
Merged

Conversation

muhrin
Copy link
Collaborator

@muhrin muhrin commented Oct 13, 2022

The release of kiwipy v0.8.0 made some breaking changes to the intended API of RmqThreadCommunicator. I'm putting back tests that check for the intended API which will be fixed in due time.

The release of kiwipy v0.8.0 made some breaking changes to the intended
API of RmqThreadCommunicator.  I'm putting back tests that check for the
intended API which will be fixed in due time.
@muhrin muhrin requested a review from sphuber October 13, 2022 18:37
@sphuber
Copy link
Collaborator

sphuber commented Oct 13, 2022

Apologies captain, thanks for jumping in. Anyway had to quickly make a 0.8.1 to unpin some very strict dependency requirements

@muhrin
Copy link
Collaborator Author

muhrin commented Oct 13, 2022

Nae dramas, I think it'll be a quick fix.

pytray 0.3.4 contains fix for async_ctx that was causing thread
communicator tests to fail.

Also modernised some async calls, getting rid of all async_generator
syntax (not needed as our python minimum is 3.7)
@muhrin muhrin changed the title [WIP] Reinstanting RmqThreadCommunicator tests Reinstanting RmqThreadCommunicator tests Oct 20, 2022
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 89.54% // Head: 90.20% // Increases project coverage by +0.67% 🎉

Coverage data is based on head (5f503f1) compared to base (50c729a).
Patch coverage: 90.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #124      +/-   ##
===========================================
+ Coverage    89.54%   90.20%   +0.67%     
===========================================
  Files           15       15              
  Lines         1137     1132       -5     
===========================================
+ Hits          1018     1021       +3     
+ Misses         119      111       -8     
Impacted Files Coverage Δ
src/kiwipy/rmq/threadcomms.py 93.72% <50.00%> (+5.67%) ⬆️
src/kiwipy/rmq/tasks.py 94.96% <100.00%> (-0.56%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@muhrin
Copy link
Collaborator Author

muhrin commented Oct 20, 2022

Ok. I've fixed the problem. Turned out to be an upstream bug in pytray.LoopScheduler. Long story short, it was propagating a future the wrong way around: instead of the result from a thread future being passed to the asyncio future, it was doing the opposite. Ultimately this meant that the RmqIncomingTask.processing() context would never have a value in the future it was waiting on, and therefore assume that the task should just be requeued.

All fixed in pytray 0.3.4.

Copy link
Collaborator

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot @muhrin ! Just a few minor things to cleanup and then we can merge this.

@sphuber sphuber self-requested a review October 20, 2022 09:02
Copy link
Collaborator

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Minor changes

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@sphuber sphuber merged commit 6deab76 into aiidateam:develop Oct 20, 2022
sphuber pushed a commit that referenced this pull request Oct 20, 2022
In commit 3806a9e the requirement of the
`aio-pika` dependency was updated. In adjusting the code to the changes
in their API, certain methods of the `RmqThreadCommunicator` were made
asynchronous. However, the whole point of the `RmqThreadCommunicator` is
to shield the consumer from any async code.

The changes in the interface and the tests are reverted and the
implementation of `RmqThreadCommunicator` is updated to match the inter-
face of the newer `aio-pika` version.

Doing so actually revealed a bug in `pytray` causing the unit test
`test_queue_get_next` to hang forever while waiting for the future to be
set. The `pytray.LoopScheduler` was propagating a future the wrong way
around: instead of the result from a thread future being passed to the
asyncio future, it was doing the opposite causing the future to never be
resolved. The bug is fixed in `pytray==0.3.4`.
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.

None yet

2 participants