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

pamqp dependency necessary? #112

Open
ltalirz opened this issue Aug 4, 2022 · 3 comments
Open

pamqp dependency necessary? #112

ltalirz opened this issue Aug 4, 2022 · 3 comments

Comments

@ltalirz
Copy link
Member

ltalirz commented Aug 4, 2022

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.

@sphuber @chrisjsewell @muhrin Is this dependency really necessary for this check?
Or could it be replaced by checking against some type in one of the other packages we also depend on?

@sphuber
Copy link
Collaborator

sphuber commented Aug 4, 2022

Looking at the history it seems that @muhrin added it so I don't know whether it can be removed. Note, however, that aiida-core also depends on pamqp~=2.3 at the moment. This is to provide support for some older versions of RabbitMQ. See this commit: aiidateam/aiida-core@716a1d8

@muhrin
Copy link
Collaborator

muhrin commented Aug 5, 2022

@ltalirz , just to answer the question on whether this check is necessary. Yes, we do want it, as the broadcast_send can fail for a variety of reasons and the only way to know is whether the returned type is a Basic.Ack. I think I've come across APIs returning subclasses of this, hence the isinstance. What would be ideal, of course, is if aio_pika abstracted this away and had a function that answers the question of 'Did the send call succeed?' (which internally could do an isinstance check if it wanted to).

@unkcpz
Copy link
Member

unkcpz commented Aug 6, 2022

I have a look at aiormq code the Ack is returned from _confirm_delivery of https://github.com/mosquito/aiormq/blob/ded8f83644963d93a94c0653c07d431d6ff50a67/aiormq/channel.py#L347-L369
It is either an Ack or None, so we can change the check to return result is not None.
If you are all good with it I'll make a PR.

nevermind, I was wrong about it, the result of broadcast_send is not the return of this _confirm_delivery. But then it is either an Ack set to result or a fut exception which maybe can be checked by a try..except.

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

No branches or pull requests

4 participants