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: lock down aio-pika dependency #110

Merged
merged 9 commits into from
Aug 5, 2022
Merged

fix: lock down aio-pika dependency #110

merged 9 commits into from
Aug 5, 2022

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Aug 4, 2022

fixes #94
fixes #111
see also #112

CI tests fail with recent versions of aio-pika (e.g. version 8) due to API changes.

Furthermore, the Jupyter notebook test from the test suite seems to reproducibly fail with aio-pika version 6.8.2 but pass with aio-pika 6.8.1 (see commits in PR).

Also, kiwipy depends directly on pamqp because of this single check:

return isinstance(result, pamqp.specification.Basic.Ack)

This dependency was not made explicit in the setup.py, and the pre-commit pylint checks alert us to the fact that the API of pamqp 3 has changed.

@ltalirz ltalirz marked this pull request as draft August 4, 2022 10:49
@ltalirz
Copy link
Member Author

ltalirz commented Aug 4, 2022

Hm... @unkcpz looks like the jupyter notebook test now fails across all python/rabbitmq versions.

It's not clear to me what changed - the last passing commit is from Jan 17 2022.
At that point, the latest aio-pika release was 6.8.1 (here we're now installing 6.8.2); I can't really imagine that this is the difference?

The only other thing I notice is that aio-pika 6 is using a somewhat dated version of aiormq (3.3.1) from 2020. Today, aiormq 6 is already out.

try to reproduce passing CI from January 2022.
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #110 (915fdda) into develop (adf373e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #110   +/-   ##
========================================
  Coverage    90.32%   90.32%           
========================================
  Files           16       16           
  Lines         1146     1146           
========================================
  Hits          1035     1035           
  Misses         111      111           

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

@ltalirz
Copy link
Member Author

ltalirz commented Aug 4, 2022

Wow - either the problem was introduced in aio-pika 6.8.2 or the test is not reproducible.

ltalirz added 2 commits August 4, 2022 04:34
just to trigger another CI run; let's see whether it still works
let's see whether jupyter notebook test fails reproducibly with aio-pika 6.8.2
@ltalirz ltalirz changed the title test rmq 3.7 lock-down aio-pika dependency Aug 4, 2022
@ltalirz ltalirz marked this pull request as ready for review August 4, 2022 12:07
@ltalirz
Copy link
Member Author

ltalirz commented Aug 4, 2022

@sphuber @chrisjsewell
Could one of you review this please?

@ltalirz ltalirz changed the title lock-down aio-pika dependency fix: lock down aio-pika dependency Aug 4, 2022
@ltalirz
Copy link
Member Author

ltalirz commented Aug 4, 2022

Just for the record, the jupyter notebook test also fails with the combination aio-pika 6.8.2 and pamqp 2 (which was not tested in this PR).

Copy link
Member

@unkcpz unkcpz 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.

I am prepare the PR #113 moving aio-pika to 8.x.
@ltalirz If I understand correctly, we need have this PR merged and release a new version and then have aio-pika=8.x support another new release.

@ltalirz
Copy link
Member Author

ltalirz commented Aug 5, 2022

Correct

@sphuber
Copy link
Collaborator

sphuber commented Aug 5, 2022

Do you guys mean that this PR with upper limit <6.8.2 needs a patch release and then #113 with ~=8.0 should be a new minor release?

@ltalirz
Copy link
Member Author

ltalirz commented Aug 5, 2022

Yes (upper limit is both for aio-pika and for pamqp, both of which currently break kiwipy)

@sphuber
Copy link
Collaborator

sphuber commented Aug 5, 2022

Ok great, thanks. Want me to take care of merging and making a release?

@ltalirz
Copy link
Member Author

ltalirz commented Aug 5, 2022

That would be great, thanks Seb!

@sphuber sphuber self-requested a review August 5, 2022 12:14
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 @ltalirz

@sphuber sphuber merged commit 7750921 into develop Aug 5, 2022
@sphuber sphuber deleted the test-rmq-3.7 branch August 5, 2022 12:16
@sphuber
Copy link
Collaborator

sphuber commented Aug 5, 2022

v0.7.6 is now released. By the way, just noticed that this fixes #94 👍

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.

CI tests fail with aio-pika 6.8.2 (and above) Dependencies: pamqp is not explicitly declared as dependency
3 participants