aiidateam / kiwipy

A python messaging library for RPC, task queues and broadcasts
https://kiwipy.readthedocs.io
Other
15 stars 4 forks source link

pamqp dependency necessary? #112

Open ltalirz opened 1 year ago

ltalirz commented 1 year ago

kiwipy depends directly on pamqp because of this single check:

https://github.com/aiidateam/kiwipy/blob/adf373e794ed69d5ec21d4875514971f32d7734f/kiwipy/rmq/threadcomms.py#L261

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 commented 1 year ago

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: https://github.com/aiidateam/aiida-core/commit/716a1d8f6801bd9ba72162a73497e1085a64bc52

muhrin commented 1 year ago

@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 commented 1 year ago

~~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.