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

add queueing and multi process support #48

Merged
merged 11 commits into from
Dec 12, 2023

Conversation

Peddle
Copy link
Contributor

@Peddle Peddle commented Dec 7, 2023

  • Adds ability to queue multiple requests for better local dev experience
  • adds request id to all logs
  • adds ability to configure number of processes

@Peddle Peddle requested a review from ErikKaum December 7, 2023 08:50
@Peddle Peddle changed the title add queueing and multi process support WIP: add queueing and multi process support Dec 7, 2023
Copy link

@dMARLAN dMARLAN left a comment

Choose a reason for hiding this comment

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

🚗 drive by review 🚗💨

nice work, left comments 🍌

main thing is I think a lot of code can be extracted into private methods to make everything a lot cleaner to read & easier to maintain.

potassium/__init__.py Show resolved Hide resolved
potassium/potassium.py Show resolved Hide resolved
potassium/potassium.py Outdated Show resolved Hide resolved
potassium/potassium.py Outdated Show resolved Hide resolved
potassium/potassium.py Show resolved Hide resolved
potassium/types.py Show resolved Hide resolved
potassium/worker.py Show resolved Hide resolved
potassium/worker.py Show resolved Hide resolved
potassium/worker.py Show resolved Hide resolved
potassium/worker.py Show resolved Hide resolved
potassium/potassium.py Outdated Show resolved Hide resolved
@Peddle Peddle changed the title WIP: add queueing and multi process support add queueing and multi process support Dec 8, 2023
@Peddle Peddle requested a review from ErikKaum December 8, 2023 02:04
@erik-dunteman
Copy link
Contributor

Doing some QA.

I am just observing behavior, not saying we should fix anything just yet.

@erik-dunteman
Copy link
Contributor

If I run single worker, three background tasks w/ 5s sleep, they all queue (nice) but the 200 responses all log under the first request id.
Screen Shot 2023-12-07 at 6 59 24 PM

@erik-dunteman
Copy link
Contributor

erik-dunteman commented Dec 8, 2023

Worker ID super helpful in logs. The order they consume from the queue makes sense. 3 bg calls to 2 workers, first 2 calls claimed immediately, third claimed by the worker that took # 1
Screen Shot 2023-12-07 at 7 02 43 PM

@erik-dunteman
Copy link
Contributor

Going to try with 100k processes, wish me luck

@erik-dunteman
Copy link
Contributor

Got up to 32 workers (BERT) running locally, laptop struggled but seemingly no bugs.

I'd suggest

  • cap quantity of workers so users don't footgun and Banana doesn't get loaded up with OOM-bombs
  • showing some proof of a highly concurrent slam test passing

@erik-dunteman
Copy link
Contributor

Tentative LGTM from me given above is addressed

@Peddle Peddle merged commit 5d7ffe1 into main Dec 12, 2023
2 checks passed
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.

4 participants