8000 feat: Migrate NATS Queue to Rust (#669) by jthomson04 · Pull Request #961 · ai-dynamo/dynamo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Migrate NATS Queue to Rust (#669) #961

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 6, 2025
Merged

feat: Migrate NATS Queue to Rust (#669) #961

merged 4 commits into from
May 6, 2025

Conversation

jthomson04
Copy link
Contributor

Overview:

Migrate the NATS queue in examples/llm/utils/nats_queue.py from Python to Rust

Details:

  1. Move NATSQueue logic to Rust, in lib/runtime/transports/nats.rs
  2. Update the examples/llm example to use this new rust-based queue
  3. Add bindings in lib/bindings

Related Issues:

grahamking
grahamking reviewed May 6, 2025
View reviewed changes
@jthomson04 jthomson04 force-pushed the jothomson/nats-queue-to-rust branch from d085638 to b5bb564 Compare May 6, 2025 19:45
@tedzhouhk
Copy link
Contributor
tedzhouhk commented May 6, 2025 •
edited
Loading
8000

Thanks for taking this task and nice work!
Just one thing, have we benchmark python vs rust prefill queue performance? The implementation in this PR still leaves the serialization of RemotePrefillRequest in python using msgspec. Wonder if that might become the bottleneck and if we want to move that also to rust (maybe can use a general dictionary instead of a pydantic class?)

@jthomson04 jthomson04 force-pushed the jothomson/nats-queue-to-rust branch from b5bb564 to 74f3774 Compare May 6, 2025 20:58
@jthomson04
Copy link
Contributor Author

Thanks for taking this task and nice work! Just one thing, have we benchmark python vs rust prefill queue performance? The implementation in this PR still leaves the serialization of RemotePrefillRequest in python using msgspec. Wonder if that might become the bottleneck and if we want to move that also to rust (maybe can use a general dictionary instead of a pydantic class?)

For now, there aren't any benchmarks. It looks like the python msgpack using cpython bindings, so I wouldn't expect it to be too much slower than a rust implementation, but it certainly has the potential to be a bottleneck on long context. I'll make a follow-on MR to investigate doing the serialization/deserialization in Rust.

@jthomson04 jthomson04 merged commit c421389 into main May 6, 2025
12 checks passed
@jthomson04 jthomson04 deleted the jothomson/nats-queue-to-rust branch May 6, 2025 22:36
@grahamking
Copy link
Contributor
grahamking commented May 15, 2025 •
edited
Loading

@jthomson04 Do the messages need to be ACKed when taken off the queue?

Details in

@jthomson04
Copy link
Contributor Author

@jthomson04 Do the messages need to be ACKed when taken off the queue?

Details in

Yes, they're acked:

dynamo/lib/runtime/src/transports/nats.rs

Line 460 in 889ab67

.ack()

@grahamking
Copy link
Contributor

Yes, they're acked:

dynamo/lib/runtime/src/transports/nats.rs

Line 460 in 889ab67

.ack()

Darn. That means the ACK is not sufficient to take them off the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamking grahamking grahamking approved these changes

@tedzhouhk tedzhouhk tedzhouhk approved these changes

@hutm hutm Awaiting requested review from hutm hutm is a code owner

@biswapanda biswapanda Awaiting requested review from biswapanda biswapanda is a code owner

@ishandhanani ishandhanani Awaiting requested review from ishandhanani ishandhanani is a code owner

@julienmancuso julienmancuso Awaiting requested review from julienmancuso julienmancuso is a code owner

@hhzhang16 hhzhang16 Awaiting requested review from hhzhang16 hhzhang16 is a code owner

@nnshah1 nnshah1 Awaiting requested review from nnshah1 nnshah1 is a code owner

@tanmayv25 tanmayv25 Awaiting requested review from tanmayv25 tanmayv25 is a code owner

@piotrm-nvidia piotrm-nvidia Awaiting requested review from piotrm-nvidia piotrm-nvidia is a code owner

@ptarasiewiczNV ptarasiewiczNV Awaiting requested review from ptarasiewiczNV ptarasiewiczNV is a code owner

@ryanolson ryanolson Awaiting requested review from ryanolson ryanolson is a code owner

@paulhendricks paulhendricks Awaiting requested review from paulhendricks paulhendricks is a code owner

@tmonty12 tmonty12 Awaiting requested review from tmonty12 tmonty12 is a code owner

@GuanLuo GuanLuo Awaiting requested review from GuanLuo GuanLuo is a code owner

@rmccorm4 rmccorm4 Awaiting requested review from rmccorm4 rmccorm4 is a code owner

@alec-flowers alec-flowers Awaiting requested review from alec-flowers alec-flowers is a code owner

@kkranen kkranen Awaiting requested review from kkranen kkranen is a code owner

@sshchoi sshchoi Awaiting requested review from sshchoi sshchoi is a code owner

@PeaBrane PeaBrane Awaiting requested review from PeaBrane PeaBrane is a code owner

@mohammedabdulwahhab mohammedabdulwahhab Awaiting requested review from mohammedabdulwahhab mohammedabdulwahhab is a code owner

@oandreeva-nv oandreeva-nv Awaiting requested review from oandreeva-nv oandreeva-nv is a code owner

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jthomson04 @tedzhouhk @grahamking
0