feat(nvtx): add context server routes - #563
Conversation
28ab44a to
e072060
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe query-engine server now reports engine context resources and merges integration routes. A new NVTX server imports event streams, reconstructs and caches models, and exposes catalog and viewport endpoints with validation and error handling. ChangesEngine context ownership and query routing
NVTX server
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The PR adds context-scoped NVTX catalog and viewport routes with request limits and caching; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@domains/query_engine/server/src/analyzer_cache.rs`:
- Around line 235-241: Make AnalyzerCache::contexts asynchronous and execute
self.lister inside tokio::task::spawn_blocking, propagating join and lister
errors consistently with list_with_metadata and get. Update engine_contexts to
await contexts while preserving the existing EngineContexts construction and
results.
In `@integrations/nvtx/server/src/lib.rs`:
- Around line 115-153: Add a shared Tokio Semaphore to NvtxModelCache and
acquire a permit inside the async initialization closure used by
NvtxModelCache::get before spawn_blocking. Move the permit into the blocking
task so imports, NvtxModelBuilder::build, and subsequent viewport work remain
bounded across distinct context IDs; preserve existing error propagation and
cache behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 12d46dee-bf1d-406e-8ba2-1bd9547b79fb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (7)
Cargo.tomldomains/query_engine/server/src/analyzer_cache.rsdomains/query_engine/server/src/lib.rsdomains/query_engine/server/src/ui.rsdomains/query_engine/ui/src/lib.rsintegrations/nvtx/server/Cargo.tomlintegrations/nvtx/server/src/lib.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
domains/query_engine/server/src/ui.rs (1)
359-359: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the missing
@quent/clientcontract. Define and export a typed fetcher andqueryOptionsforGET /api/engines/{engine_id}/contexts, withengine_idandcontext_resources: BTreeMap<Uuid, Vec<Uuid>>in the response type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domains/query_engine/server/src/ui.rs` at line 359, Define and export the `@quent/client` contract for GET /api/engines/{engine_id}/contexts, including a typed fetcher and queryOptions. Model the response with engine_id and context_resources as BTreeMap<Uuid, Vec<Uuid>>, and align the endpoint path and parameter typing with the existing client contract conventions.Source: Path instructions
🧹 Nitpick comments (1)
domains/query_engine/server/src/ui.rs (1)
137-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd endpoint tests for
GET /api/engines/{engine_id}/contexts.Cover a non-empty JSON response, an unknown
engine_idreturning an emptycontext_resourcesmap, and a lister failure returning HTTP 500.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domains/query_engine/server/src/ui.rs` at line 137, Add endpoint tests for the `GET /api/engines/{engine_id}/contexts` handler around the `state.analyzers.contexts(engine_id)` call, covering a non-empty JSON response, an unknown engine that returns an empty `context_resources` map, and a lister failure that returns HTTP 500.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@domains/query_engine/server/src/ui.rs`:
- Line 359: Define and export the `@quent/client` contract for GET
/api/engines/{engine_id}/contexts, including a typed fetcher and queryOptions.
Model the response with engine_id and context_resources as BTreeMap<Uuid,
Vec<Uuid>>, and align the endpoint path and parameter typing with the existing
client contract conventions.
---
Nitpick comments:
In `@domains/query_engine/server/src/ui.rs`:
- Line 137: Add endpoint tests for the `GET /api/engines/{engine_id}/contexts`
handler around the `state.analyzers.contexts(engine_id)` call, covering a
non-empty JSON response, an unknown engine that returns an empty
`context_resources` map, and a lister failure that returns HTTP 500.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 6412b423-0bb8-4adf-af65-8cda2154ce62
📒 Files selected for processing (4)
domains/query_engine/server/src/analyzer_cache.rsdomains/query_engine/server/src/ui.rsdomains/query_engine/ui/src/lib.rsintegrations/nvtx/server/src/lib.rs
💤 Files with no reviewable changes (1)
- domains/query_engine/ui/src/lib.rs
Thank you for the thorough reviews :) |
|
/merge |
## Summary - mount the context-scoped NVTX routes in simulator and generated quent-open viewers - export engine-context and NVTX bindings through the UI packages - add client fetchers, query options, and hooks for engine contexts, catalogs, and viewports - canonicalize selector inputs, include query_start in requests and cache keys, and preserve the relative-second contract types Follow up of #563 ## Verification - pixi run pnpm --dir ui typecheck - pixi run pnpm --dir ui lint - focused NVTX client and resource-tree tests - relevant Rust viewer, simulator, and server test suites as part of the reconciled wave stack Authors: - Pradeep Garigipati (https://github.com/9prady9) Approvers: - Joe O'Hallaron (https://github.com/johallar) - Joost Hoozemans (https://github.com/joosthooz) URL: #571
Summary
The routes use the viewport contracts introduced in #562. A required
query_startparameter establishes the time origin for catalog and viewport responses.Verification
pixi run cargo test -p nvtx-server -p quent-query-engine-serverpixi run cargo clippy -p nvtx-server -p quent-query-engine-server --all-targets -- -D warningscargo fmt --all -- --check