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
Add RabbitMQ Queue #18
Conversation
src/saturn/worker/services/config.py
Outdated
from argparse import Namespace | ||
|
||
|
||
class ConfigService: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we use this? --url=
?
How about using SATURN_AMQP_URL?
Also, do you have suggetions for configuring the worker manager's database URL?
We could have a PR that introduces configuration loading/management to make this one smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a suggestion for the worker manager:
-> #19
src/saturn/worker/queues/__init__.py
Outdated
pass | ||
|
||
async def get(self) -> T: | ||
pass | ||
async def iterator(self) -> AsyncGenerator[Message, None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I prefer to use AsyncIterator[Message]
when there is no send type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using aclose()
on these returned value which is a method on AsyncGenerator
!
src/saturn/worker/queues/__init__.py
Outdated
pass | ||
async def iterator(self) -> AsyncGenerator[Message, None]: | ||
for _ in (): | ||
yield _ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to raise instead? Or mypy was complaining that there was no yield?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a yield otherwise if won't be a generator :/
async for item, _ in alib.zip(scheduler.iter(), range(10)): | ||
scheduler_iter = scheduler.iter() | ||
|
||
async for item, _ in alib.zip(alib.borrow(scheduler_iter), range(10)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
today I learned about borrowing iterators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be something from asyncstdlib
with no equivalent in the stdlib. In any case, this is useful here to avoid zip
closing the original generator. (I might have spent way more time than I wish debugging the initial code with repeated call to scheduler.iter()
)
looks good to me. Feel free to merge even if that not all you want to get done. As long as your tests pass we can merge this and move on with other PRs. |
b9256b5
to
68d27bc
Compare
No description provided.