fix: Httpengine sync-enable-endpoint #2591
Conversation
|
👋 Hi michaelfeil! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
the
external-contribution
michaelfeil
changed the title
WalkthroughAdds public re-export of EndpointType, a new Python-exposed HttpService.enable_endpoint, and a Rust HttpService.sync_enable_model_endpoint. The Python method maps string types to EndpointType, then calls the synchronous helper, which updates endpoint flags and logs. The async enable_model_endpoint now delegates to the sync helper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python Caller
participant PySvc as Py HttpService
participant RsSvc as Rust HttpService (v2)
participant Flags as StateFlags
Py->>PySvc: enable_endpoint(endpoint_type: str, enabled: bool)
alt valid type ("chat"/"completion"/"embedding")
PySvc->>PySvc: Map str -> EndpointType
PySvc->>RsSvc: sync_enable_model_endpoint(EndpointType, enabled)
RsSvc->>Flags: set(endpoint_type, enabled)
Flags-->>RsSvc: updated
RsSvc-->>PySvc: Ok(())
PySvc-->>Py: Ok(())
else invalid type
PySvc-->>Py: Err(Invalid endpoint_type)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/llm/src/http/service/service_v2.rs (1)
269-276: Consider stronger atomic ordering (Acquire/Release or SeqCst) for endpoint enable flags.Using Relaxed loads/stores in StateFlags means other threads may observe stale values briefly. For an on/off gate evaluated in request middleware, stronger ordering can be simpler and safer with negligible cost. If you prefer Relaxed, add a short comment documenting the rationale.
Apply this diff to switch to SeqCst in get/set (referenced here; edits occur in StateFlags::get and StateFlags::set above):
- EndpointType::Chat => self.chat_endpoints_enabled.load(Ordering::Relaxed), - EndpointType::Completion => self.cmpl_endpoints_enabled.load(Ordering::Relaxed), - EndpointType::Embedding => self.embeddings_endpoints_enabled.load(Ordering::Relaxed), - EndpointType::Responses => self.responses_endpoints_enabled.load(Ordering::Relaxed), + EndpointType::Chat => self.chat_endpoints_enabled.load(Ordering::SeqCst), + EndpointType::Completion => self.cmpl_endpoints_enabled.load(Ordering::SeqCst), + EndpointType::Embedding => self.embeddings_endpoints_enabled.load(Ordering::SeqCst), + EndpointType::Responses => self.responses_endpoints_enabled.load(Ordering::SeqCst),- EndpointType::Chat => self - .chat_endpoints_enabled - .store(enabled, Ordering::Relaxed), - EndpointType::Completion => self - .cmpl_endpoints_enabled - .store(enabled, Ordering::Relaxed), - EndpointType::Embedding => self - .embeddings_endpoints_enabled - .store(enabled, Ordering::Relaxed), - EndpointType::Responses => self - .responses_endpoints_enabled - .store(enabled, Ordering::Relaxed), + EndpointType::Chat => self + .chat_endpoints_enabled + .store(enabled, Ordering::SeqCst), + EndpointType::Completion => self + .cmpl_endpoints_enabled + .store(enabled, Ordering::SeqCst), + EndpointType::Embedding => self + .embeddings_endpoints_enabled + .store(enabled, Ordering::SeqCst), + EndpointType::Responses => self + .responses_endpoints_enabled + .store(enabled, Ordering::SeqCst),Optional: return the previous state from sync_enable_model_endpoint for observability, but that would change the signature and call sites.
lib/bindings/python/rust/http.rs (1)
22-22: Re-exporting EndpointType is fine for Rust usage; consider a Python Enum to avoid stringly-typed API.Right now Python callers must pass strings. Exposing a Python-visible enum (or simple constants) would give call-time validation and autocompletion. Not required for this PR, but improves ergonomics and reduces typos.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/bindings/python/rust/http.rs(2 hunks)lib/llm/src/http/service/service_v2.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/bindings/python/rust/http.rs (1)
lib/bindings/python/rust/lib.rs (1)
to_pyerr(124-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (1)
lib/llm/src/http/service/service_v2.rs (1)
265-268: Good: async wrapper preserves public API while delegating to sync helper.This keeps the previous async signature intact and centralizes the logic in a sync helper, which unblocks non-async call sites (e.g., Python binding). Looks good.
I don't understand that. Could you re-phrase? |
|
Rephrased @grahamking |
|
@grahamking Modified as you requested. |
Signed-off-by: Hannah Zhang <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: nnshah1 <[email protected]>
2 participants
Overview:
Details:
Playing back a PR from upgrading dynamo from 0.1 to 0.4.
Re-enabled the broken usage of HTTPEngine. Currently, the HTTPEngine can't receive any requests (they get returned with status code 503), because the flag to enable the given engine can't be enabled.
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Refactor