Move simulator from examples/ to domains/ - #568
Draft
erensh27 wants to merge 1 commit into
Draft
Conversation
The simulator is a query engine simulator, not an example, so it belongs under domains/ next to query_engine. Updated workspace members, Cargo path dependencies, docs, pre-commit exclusions, and CI base-branch paths. Fixes rapidsai#102
erensh27
requested review from
9prady9,
cmatzenbach,
dhruv9vats,
felipeblazing,
johallar,
johanpel,
joosthooz and
mbrobbel
as code owners
August 11, 2026 10:29
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThe simulator moves from ChangesSimulator domain
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
domains/simulator/server/src/main.rs-64-68 (1)
64-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the environment variable name in the help text.
Line 66 states
QUENT_CORS_ADDRESS. Line 34 defines the actual variable asQUENT_ANALYZER_CORS_ADDRESS, and line 67 binds that name. Users read line 66 in--helpoutput and set a variable that has no effect.🐛 Proposed fix
/// Address to allow CORS requests from (e.g. "http://localhost:5173"). /// If not set, CORS is disabled. - /// Overridden by the QUENT_CORS_ADDRESS environment variable if set. + /// Overridden by the QUENT_ANALYZER_CORS_ADDRESS environment variable if set. #[arg(long, env = env::QUENT_ANALYZER_CORS_ADDRESS)] cors_address: Option<String>,🤖 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/simulator/server/src/main.rs` around lines 64 - 68, Update the CORS configuration documentation above the cors_address argument to reference QUENT_ANALYZER_CORS_ADDRESS consistently with the env binding and its definition, so the --help text names the effective environment variable.
🧹 Nitpick comments (6)
domains/simulator/instrumentation/Cargo.toml (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the sibling-domain path dependency.
Line 12 travels to the repository root and back into
domains/. The equivalent direct path is../../query_engine/model. This matches how line 12 indomains/simulator/analyzer/Cargo.tomland the../instrumentationentries express sibling paths.♻️ Proposed path simplification
-quent-query-engine-model = { path = "../../../domains/query_engine/model" } +quent-query-engine-model = { path = "../../query_engine/model" }🤖 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/simulator/instrumentation/Cargo.toml` around lines 11 - 13, Update the quent-query-engine-model dependency in the instrumentation Cargo.toml manifest to use the direct sibling-domain path ../../query_engine/model instead of traversing through the repository root; leave the other dependency paths unchanged.domains/simulator/analyzer/Cargo.toml (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the sibling-domain path dependencies.
Lines 11-13 travel to the repository root and back into
domains/. The direct sibling paths are../../query_engine/analyzer,../../query_engine/model, and../../query_engine/ui. The same file already uses direct relative paths for../instrumentationand../ui.🤖 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/simulator/analyzer/Cargo.toml` around lines 11 - 13, Update the path dependencies for quent-query-engine-analyzer, quent-query-engine-model, and quent-query-engine-ui to use the direct sibling paths ../../query_engine/analyzer, ../../query_engine/model, and ../../query_engine/ui, matching the existing relative-path style in Cargo.toml.domains/simulator/application/Cargo.toml (1)
7-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExternal crate versions are pinned per manifest instead of in
[workspace.dependencies]. Both new manifests declare literal versions for external crates while declaringpetgraph,tokio,tracing, anduuidwithworkspace = true. The result is two differentclapconstraints inside one workspace, and version bumps must touch each manifest separately.
domains/simulator/application/Cargo.toml#L7-L16: replaceclap = {version = "4.5.57", ...}andrand = "0.10.2"withworkspace = trueentries, and add both crates to[workspace.dependencies]in the rootCargo.toml.domains/simulator/server/Cargo.toml#L11-L12: replaceaxum = { version = "0.8.7" }andclap = { version = "4.5", ... }withworkspace = trueentries that reuse the same root declarations.🤖 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/simulator/application/Cargo.toml` around lines 7 - 16, Centralize the external dependency versions in the root workspace: add clap and rand to [workspace.dependencies], then update domains/simulator/application/Cargo.toml lines 7-16 to use workspace = true for both; also update domains/simulator/server/Cargo.toml lines 11-12 so axum and clap use the shared workspace declarations.domains/simulator/application/src/main.rs (1)
748-986: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the per-operator statistics generation.
execute_logical_plannow spans lines 647-986. Roughly 240 of those lines are thematch op.kindblock that builds random statistics. This block has one purpose and no dependency on worker state.Move lines 749-965 into a free function, for example
fn operator_statistics(kind: Physical, tasks_processed: u64) -> Vec<DynamicAttribute>.execute_logical_planthen reads as plan declaration, execution, and statistics emission.🤖 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/simulator/application/src/main.rs` around lines 748 - 986, Extract the random per-operator attribute construction, including the attr macro and match on Physical, from execute_logical_plan into a free function such as operator_statistics(kind: Physical, tasks_processed: u64) -> Vec<DynamicAttribute>. Keep all existing statistic generation behavior unchanged, then call this function when populating operator::Statistics so execute_logical_plan only handles plan execution and emission.domains/simulator/analyzer/src/model.rs (1)
471-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMutate the model instead of rebuilding it field by field.
Lines 473-501 build
temp_model, then destructure every field into a secondSimulatorModelonly to attachresource_group_types. A new field onSimulatorModelmust be added in both places.♻️ Proposed simplification
- let temp_model = SimulatorModel { + let mut model = SimulatorModel { query_engine, arbitrary_resources: resources, tasks, resource_group_types: HashMap::default(), }; - let mut resource_group_types = derive_resource_group_types(&temp_model)?; + let mut resource_group_types = derive_resource_group_types(&model)?; // Bubble up all the used_by_entity fields in the group type decls. for group_type_decl in resource_group_types.values_mut() { for contained_resource_type in &group_type_decl.contains_resource_types { - if let Ok(resource_type) = temp_model + if let Ok(resource_type) = model .arbitrary_resources .resource_type(contained_resource_type) { @@ - Ok(SimulatorModel { - query_engine: temp_model.query_engine, - arbitrary_resources: temp_model.arbitrary_resources, - tasks: temp_model.tasks, - resource_group_types, - }) + model.resource_group_types = resource_group_types; + Ok(model)🤖 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/simulator/analyzer/src/model.rs` around lines 471 - 501, Mutate the existing temp_model instead of constructing a second SimulatorModel: make temp_model mutable, assign the derived and updated resource_group_types to its field after the bubbling loop, then return temp_model directly. Update the construction flow around SimulatorModel and derive_resource_group_types while preserving all existing field values.domains/simulator/analyzer/src/lib.rs (1)
657-684: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant borrow and second index.
Line 663 binds
entrywith&mutbut only readsentry.operator_ids. Line 665 then indexesplain_buildersagain. The same pattern repeats on lines 675-679, 811-813, and 822-826.Read the filter once, then index once.
♻️ Proposed simplification
if let Some(builder_indices) = plain_index.get(&resource_id) { for &builder_idx in builder_indices { - let entry = &mut plain_builders[builder_idx]; - if operator_matches(&entry.operator_ids, task_operator_id) { - plain_builders[builder_idx].builder.try_push(&usage)?; + let entry = &mut plain_builders[builder_idx]; + if operator_matches(&entry.operator_ids, task_operator_id) { + entry.builder.try_push(&usage)?; } } }If the borrow checker rejects the combined borrow, split the filter check first:
let matches = operator_matches(&plain_builders[builder_idx].operator_ids, task_operator_id); if matches { plain_builders[builder_idx].builder.try_push(&usage)?; }🤖 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/simulator/analyzer/src/lib.rs` around lines 657 - 684, In the task usage processing loops, remove the mutable entry borrow and avoid indexing each builder twice: read the operator IDs from the indexed builder, store the match result, then perform a single mutable builder access for try_push. Apply this pattern to both plain and per-state builders here and to the corresponding patterns around the other reported locations.
🤖 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 @.github/workflows/ui.yml:
- Line 177: Update the fallback binding path assigned to base_bindings in the
workflow’s pre-relocation branch to use
base/examples/simulator/server/ts-bindings, while preserving the ui-bindings
path when it exists.
In `@crates/codegen/Cargo.toml`:
- Line 18: Add quent-simulator-instrumentation to the root workspace
dependencies table, then update its dev-dependency declaration in the crate
manifest to use the workspace dependency instead of the local path.
In `@domains/simulator/analyzer/src/lib.rs`:
- Around line 1069-1094: Refactor SimulatorUiAnalyzer::entities_filtered to
validate entity_filter.entity_type_name first, returning the existing
InvalidArgument error for unknown types, then construct a single
tasks.values().filter closure using operator_matches and the time-window span
check. Preserve the current behavior for the recognized "task" type and for
filters without an entity type.
- Around line 1-140: Add accompanying tests across the simulator crates: in
domains/simulator/analyzer/src/lib.rs:1-140, table-test
operator_statistic_quantity, scale_operator_statistic, and
scaled_operator_statistic_name; in
domains/simulator/instrumentation/src/task.rs:73-96, exercise the Task FSM’s
emitted sequence including multi-worker sending and exit; in
domains/simulator/analyzer/src/model.rs:437-502, test
SimulatorModelBuilder::try_push and try_build with valid events and a missing
resource initialization that returns an error; in
domains/simulator/analyzer/src/task.rs:35-96, test active_span with one, two,
and many transitions plus try_to_ui_fsm bytes_per_sec derivation including equal
timestamps; and in domains/simulator/server/src/main.rs:92-99, verify each
exporter name maps to its expected Format and unknown names error.
In `@domains/simulator/analyzer/src/model.rs`:
- Around line 258-265: Replace panic-based invariant handling with
AnalyzerResult propagation at all three sites: in
domains/simulator/analyzer/src/model.rs lines 258-265, update the
TaskBuilder::try_new entry logic to use an Entry match and propagate errors with
?; in domains/simulator/analyzer/src/model.rs lines 447-460, replace
get_mut(&resource_type_name).unwrap() with InvalidTypeName error propagation;
and in domains/simulator/analyzer/src/lib.rs lines 842-865, replace both
panic-producing unwrap_or_else calls with AnalyzerError::BrokenImpl("chunked
bulk: unknown entry id") propagated via ?.
In `@domains/simulator/application/src/main.rs`:
- Around line 701-713: Validate Args::num_threads during clap parsing so only
values of at least one are accepted, preventing an empty self.threads vector. In
the physical_plan.execute task scheduling block, replace the truncated
tasks_per_thread_per_op division with balanced range calculation that assigns
every task, including the remainder, across threads.
In `@domains/simulator/instrumentation/src/task.rs`:
- Around line 84-94: Update the task FSM transitions around JoinPartition’s
producer sequence: include sending in exit_from and add a sending => sending
self-transition so repeated sending events for other workers are accepted before
exit. Preserve the existing sending => queueing transition for the normal
completion path.
In `@domains/simulator/server/src/main.rs`:
- Around line 122-129: Update the importer closure and the
`ImporterFn`/`UiAnalyzer` interfaces so event iterators yield
`ImporterResult<Event<_>>` and propagate errors lazily. Remove the eager
`collect::<Vec<_>>()` buffering in the `Simulator::import_events` path,
preserving errors through the returned iterator without discarding them or
panicking.
In `@domains/simulator/ui-bindings/src/lib.rs`:
- Around line 19-42: Add a Rust test for generate that creates a temporary
output directory containing stale content, invokes generate, verifies generation
succeeds and the stale content is removed, and confirms expected binding files
are emitted in the requested directory. Place the test alongside generate and
use the project’s existing temporary-directory and test conventions.
In `@domains/simulator/ui/Cargo.toml`:
- Around line 1-11: Add the required SPDX copyright and Apache-2.0 license
headers before [package] in domains/simulator/ui/Cargo.toml (lines 1-11) and
domains/simulator/ui-bindings/Cargo.toml (lines 1-11). In both manifests, also
set the new crates to edition 2024 and publish = false as required by the path
instructions.
- Around line 8-11: Move the local quent-analyzer dependency from the
manifest-level path declaration into [workspace.dependencies], then reference it
with workspace = true in domains/simulator/ui/Cargo.toml:8-11. Apply the same
workspace-based declaration to all three local crates in
domains/simulator/ui-bindings/Cargo.toml:8-11, ensuring neither manifest uses
direct path or git dependency declarations.
In `@domains/simulator/ui/src/lib.rs`:
- Around line 26-32: Correct EntityRef::is_resource and
EntityRef::is_resource_group so each returns true only when self matches its
corresponding variant, Resource and ResourceGroup respectively; preserve false
for all other variants. Add unit tests covering matching and non-matching
EntityRef variants for both methods.
---
Other comments:
In `@domains/simulator/server/src/main.rs`:
- Around line 64-68: Update the CORS configuration documentation above the
cors_address argument to reference QUENT_ANALYZER_CORS_ADDRESS consistently with
the env binding and its definition, so the --help text names the effective
environment variable.
---
Nitpick comments:
In `@domains/simulator/analyzer/Cargo.toml`:
- Around line 11-13: Update the path dependencies for
quent-query-engine-analyzer, quent-query-engine-model, and quent-query-engine-ui
to use the direct sibling paths ../../query_engine/analyzer,
../../query_engine/model, and ../../query_engine/ui, matching the existing
relative-path style in Cargo.toml.
In `@domains/simulator/analyzer/src/lib.rs`:
- Around line 657-684: In the task usage processing loops, remove the mutable
entry borrow and avoid indexing each builder twice: read the operator IDs from
the indexed builder, store the match result, then perform a single mutable
builder access for try_push. Apply this pattern to both plain and per-state
builders here and to the corresponding patterns around the other reported
locations.
In `@domains/simulator/analyzer/src/model.rs`:
- Around line 471-501: Mutate the existing temp_model instead of constructing a
second SimulatorModel: make temp_model mutable, assign the derived and updated
resource_group_types to its field after the bubbling loop, then return
temp_model directly. Update the construction flow around SimulatorModel and
derive_resource_group_types while preserving all existing field values.
In `@domains/simulator/application/Cargo.toml`:
- Around line 7-16: Centralize the external dependency versions in the root
workspace: add clap and rand to [workspace.dependencies], then update
domains/simulator/application/Cargo.toml lines 7-16 to use workspace = true for
both; also update domains/simulator/server/Cargo.toml lines 11-12 so axum and
clap use the shared workspace declarations.
In `@domains/simulator/application/src/main.rs`:
- Around line 748-986: Extract the random per-operator attribute construction,
including the attr macro and match on Physical, from execute_logical_plan into a
free function such as operator_statistics(kind: Physical, tasks_processed: u64)
-> Vec<DynamicAttribute>. Keep all existing statistic generation behavior
unchanged, then call this function when populating operator::Statistics so
execute_logical_plan only handles plan execution and emission.
In `@domains/simulator/instrumentation/Cargo.toml`:
- Around line 11-13: Update the quent-query-engine-model dependency in the
instrumentation Cargo.toml manifest to use the direct sibling-domain path
../../query_engine/model instead of traversing through the repository root;
leave the other dependency paths unchanged.
🪄 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: dc4edfb3-1980-4e49-b572-9d3c258e58f3
📒 Files selected for processing (29)
.github/workflows/ui.yml.pre-commit-config.ci.yaml.pre-commit-config.yamlCargo.tomlREADME.mdcrates/codegen/Cargo.tomldocs/domains/query_engine/examples/README.mddocs/domains/query_engine/examples/simulator.mddomains/query_engine/server/Cargo.tomldomains/query_engine/tests/fixed/Cargo.tomldomains/query_engine/tests/fixed/src/lib.rsdomains/simulator/analyzer/Cargo.tomldomains/simulator/analyzer/src/lib.rsdomains/simulator/analyzer/src/model.rsdomains/simulator/analyzer/src/task.rsdomains/simulator/analyzer/src/view.rsdomains/simulator/application/Cargo.tomldomains/simulator/application/src/main.rsdomains/simulator/instrumentation/Cargo.tomldomains/simulator/instrumentation/src/lib.rsdomains/simulator/instrumentation/src/task.rsdomains/simulator/server/Cargo.tomldomains/simulator/server/src/main.rsdomains/simulator/ui-bindings/Cargo.tomldomains/simulator/ui-bindings/src/lib.rsdomains/simulator/ui-bindings/src/main.rsdomains/simulator/ui/Cargo.tomldomains/simulator/ui/src/lib.rsui/REVIEW.md
| base_bindings=base/ui/generated/ts-bindings | ||
| else | ||
| base_bindings=base/examples/simulator/server/ts-bindings | ||
| base_bindings=base/domains/simulator/server/ts-bindings |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the correct bindings path for the pre-relocation base revision.
When base/domains/simulator/ui-bindings does not exist, the workflow builds the base simulator server. On the base revision before this move, generated bindings are under base/examples/simulator/server/ts-bindings. Line 177 points to the new path, so diff cannot find the directory and the binding-change check fails.
Select the fallback from the base layout, or use the old path in this branch.
Proposed fix
if [ -d base/ui/generated/ts-bindings ]; then
base_bindings=base/ui/generated/ts-bindings
+ elif [ -d base/domains/simulator/server/ts-bindings ]; then
+ base_bindings=base/domains/simulator/server/ts-bindings
else
- base_bindings=base/domains/simulator/server/ts-bindings
+ base_bindings=base/examples/simulator/server/ts-bindings
fi🤖 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 @.github/workflows/ui.yml at line 177, Update the fallback binding path
assigned to base_bindings in the workflow’s pre-relocation branch to use
base/examples/simulator/server/ts-bindings, while preserving the ui-bindings
path when it exists.
| quent-query-engine-model = { path = "../../domains/query_engine/model" } | ||
| quent-readme-example = { path = "../../examples/readme" } | ||
| quent-simulator-instrumentation = { path = "../../examples/simulator/instrumentation" } | ||
| quent-simulator-instrumentation = { path = "../../domains/simulator/instrumentation" } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- root workspace dependency entry ---'
rg -n -C 3 '^quent-simulator-instrumentation\s*=' Cargo.toml || true
printf '%s\n' '--- target manifest ---'
cat -n crates/codegen/Cargo.toml
printf '%s\n' '--- all references ---'
rg -n 'quent-simulator-instrumentation|workspace\s*=\s*true|path\s*=' \
Cargo.toml crates --glob 'Cargo.toml' --glob '!target/**' || true
printf '%s\n' '--- manifest paths and workspace membership ---'
find crates domains -name Cargo.toml -print 2>/dev/null | sort | while read -r manifest; do
if rg -q '^name\s*=\s*"quent-simulator-instrumentation"' "$manifest"; then
printf 'crate manifest: %s\n' "$manifest"
cat -n "$manifest"
fi
doneRepository: rapidsai/quent
Length of output: 22650
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workspace and dependency tables ---'
sed -n '1,175p' Cargo.toml
printf '%s\n' '--- simulator workspace context ---'
find domains/simulator -maxdepth 3 -name Cargo.toml -print -exec sed -n '1,80p' {} \;
printf '%s\n' '--- TOML structure check ---'
python3 - <<'PY'
import tomllib
from pathlib import Path
root = tomllib.loads(Path("Cargo.toml").read_text())
target = tomllib.loads(Path("crates/codegen/Cargo.toml").read_text())
simulator = tomllib.loads(Path("domains/simulator/instrumentation/Cargo.toml").read_text())
workspace = root.get("workspace", {})
dependencies = workspace.get("dependencies", {})
members = workspace.get("members", [])
print("root workspace.dependencies contains target:",
"quent-simulator-instrumentation" in dependencies)
print("root workspace dependency value:",
dependencies.get("quent-simulator-instrumentation"))
print("target dev-dependency value:",
target.get("dev-dependencies", {}).get("quent-simulator-instrumentation"))
print("workspace member patterns containing domains:",
[member for member in members if "domain" in member or "simulator" in member])
print("simulator package name:", simulator["package"]["name"])
PYRepository: rapidsai/quent
Length of output: 9051
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
root_lines = Path("Cargo.toml").read_text().splitlines()
target_lines = Path("crates/codegen/Cargo.toml").read_text().splitlines()
simulator_manifest = Path("domains/simulator/instrumentation/Cargo.toml")
def table_ranges(lines):
ranges = {}
current = None
for index, line in enumerate(lines):
match = re.match(r"^\s*\[([^\]]+)\]\s*$", line)
if match:
current = match.group(1)
ranges.setdefault(current, []).append(index)
return ranges
root_tables = table_ranges(root_lines)
target_tables = table_ranges(target_lines)
def section(lines, table):
starts = [i for i, line in enumerate(lines)
if line.strip() == f"[{table}]"]
if not starts:
return []
start = starts[0] + 1
end = len(lines)
for i in range(start, len(lines)):
if re.match(r"^\s*\[[^\]]+\]\s*$", lines[i]):
end = i
break
return lines[start:end]
workspace_deps = section(root_lines, "workspace.dependencies")
target_dev_deps = section(target_lines, "dev-dependencies")
root_entry = next(
(line.strip() for line in workspace_deps
if re.match(r"^quent-simulator-instrumentation\s*=", line)),
None,
)
target_entry = next(
(line.strip() for line in target_dev_deps
if re.match(r"^quent-simulator-instrumentation\s*=", line)),
None,
)
print("root workspace dependency:", root_entry)
print("codegen dev-dependency:", target_entry)
print("simulator manifest exists:", simulator_manifest.is_file())
print("simulator package declaration:",
next((line.strip() for line in simulator_manifest.read_text().splitlines()
if re.match(r'^name\s*=\s*"quent-simulator-instrumentation"', line)),
None))
print("codegen entry uses workspace=true:",
bool(target_entry and re.search(r"\bworkspace\s*=\s*true\b", target_entry)))
print("codegen entry uses direct path:",
bool(target_entry and re.search(r"\bpath\s*=", target_entry)))
PYRepository: rapidsai/quent
Length of output: 477
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
root_lines = Path("Cargo.toml").read_text().splitlines()
target_lines = Path("crates/codegen/Cargo.toml").read_text().splitlines()
simulator_manifest = Path("domains/simulator/instrumentation/Cargo.toml")
def section(lines, table):
marker = f"[{table}]"
try:
start = next(i for i, line in enumerate(lines) if line.strip() == marker) + 1
except StopIteration:
return []
end = len(lines)
for i in range(start, len(lines)):
if re.match(r"^\s*\[[^\]]+\]\s*$", lines[i]):
end = i
break
return lines[start:end]
workspace_deps = section(root_lines, "workspace.dependencies")
target_dev_deps = section(target_lines, "dev-dependencies")
root_entry = next(
(line.strip() for line in workspace_deps
if re.match(r"^quent-simulator-instrumentation\s*=", line)),
None,
)
target_entry = next(
(line.strip() for line in target_dev_deps
if re.match(r"^quent-simulator-instrumentation\s*=", line)),
None,
)
print("root workspace dependency:", root_entry)
print("codegen dev-dependency:", target_entry)
print("simulator manifest exists:", simulator_manifest.is_file())
print("codegen entry uses workspace=true:",
bool(target_entry and re.search(r"\bworkspace\s*=\s*true\b", target_entry)))
print("codegen entry uses direct path:",
bool(target_entry and re.search(r"\bpath\s*=", target_entry)))
PYRepository: rapidsai/quent
Length of output: 405
Use the workspace dependency for quent-simulator-instrumentation.
Add the relocated crate to the root [workspace.dependencies] table, then change this dev-dependency to { workspace = true }.
🤖 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 `@crates/codegen/Cargo.toml` at line 18, Add quent-simulator-instrumentation to
the root workspace dependencies table, then update its dev-dependency
declaration in the crate manifest to use the workspace dependency instead of the
local path.
Source: Path instructions
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
domains/simulator/server/src/main.rs-64-68 (1)
64-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the environment variable name in the help text.
Line 66 states
QUENT_CORS_ADDRESS. Line 34 defines the actual variable asQUENT_ANALYZER_CORS_ADDRESS, and line 67 binds that name. Users read line 66 in--helpoutput and set a variable that has no effect.🐛 Proposed fix
/// Address to allow CORS requests from (e.g. "http://localhost:5173"). /// If not set, CORS is disabled. - /// Overridden by the QUENT_CORS_ADDRESS environment variable if set. + /// Overridden by the QUENT_ANALYZER_CORS_ADDRESS environment variable if set. #[arg(long, env = env::QUENT_ANALYZER_CORS_ADDRESS)] cors_address: Option<String>,🤖 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/simulator/server/src/main.rs` around lines 64 - 68, Update the CORS configuration documentation above the cors_address argument to reference QUENT_ANALYZER_CORS_ADDRESS consistently with the env binding and its definition, so the --help text names the effective environment variable.
🧹 Nitpick comments (6)
domains/simulator/instrumentation/Cargo.toml (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the sibling-domain path dependency.
Line 12 travels to the repository root and back into
domains/. The equivalent direct path is../../query_engine/model. This matches how line 12 indomains/simulator/analyzer/Cargo.tomland the../instrumentationentries express sibling paths.♻️ Proposed path simplification
-quent-query-engine-model = { path = "../../../domains/query_engine/model" } +quent-query-engine-model = { path = "../../query_engine/model" }🤖 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/simulator/instrumentation/Cargo.toml` around lines 11 - 13, Update the quent-query-engine-model dependency in the instrumentation Cargo.toml manifest to use the direct sibling-domain path ../../query_engine/model instead of traversing through the repository root; leave the other dependency paths unchanged.domains/simulator/analyzer/Cargo.toml (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the sibling-domain path dependencies.
Lines 11-13 travel to the repository root and back into
domains/. The direct sibling paths are../../query_engine/analyzer,../../query_engine/model, and../../query_engine/ui. The same file already uses direct relative paths for../instrumentationand../ui.🤖 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/simulator/analyzer/Cargo.toml` around lines 11 - 13, Update the path dependencies for quent-query-engine-analyzer, quent-query-engine-model, and quent-query-engine-ui to use the direct sibling paths ../../query_engine/analyzer, ../../query_engine/model, and ../../query_engine/ui, matching the existing relative-path style in Cargo.toml.domains/simulator/application/Cargo.toml (1)
7-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExternal crate versions are pinned per manifest instead of in
[workspace.dependencies]. Both new manifests declare literal versions for external crates while declaringpetgraph,tokio,tracing, anduuidwithworkspace = true. The result is two differentclapconstraints inside one workspace, and version bumps must touch each manifest separately.
domains/simulator/application/Cargo.toml#L7-L16: replaceclap = {version = "4.5.57", ...}andrand = "0.10.2"withworkspace = trueentries, and add both crates to[workspace.dependencies]in the rootCargo.toml.domains/simulator/server/Cargo.toml#L11-L12: replaceaxum = { version = "0.8.7" }andclap = { version = "4.5", ... }withworkspace = trueentries that reuse the same root declarations.🤖 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/simulator/application/Cargo.toml` around lines 7 - 16, Centralize the external dependency versions in the root workspace: add clap and rand to [workspace.dependencies], then update domains/simulator/application/Cargo.toml lines 7-16 to use workspace = true for both; also update domains/simulator/server/Cargo.toml lines 11-12 so axum and clap use the shared workspace declarations.domains/simulator/application/src/main.rs (1)
748-986: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the per-operator statistics generation.
execute_logical_plannow spans lines 647-986. Roughly 240 of those lines are thematch op.kindblock that builds random statistics. This block has one purpose and no dependency on worker state.Move lines 749-965 into a free function, for example
fn operator_statistics(kind: Physical, tasks_processed: u64) -> Vec<DynamicAttribute>.execute_logical_planthen reads as plan declaration, execution, and statistics emission.🤖 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/simulator/application/src/main.rs` around lines 748 - 986, Extract the random per-operator attribute construction, including the attr macro and match on Physical, from execute_logical_plan into a free function such as operator_statistics(kind: Physical, tasks_processed: u64) -> Vec<DynamicAttribute>. Keep all existing statistic generation behavior unchanged, then call this function when populating operator::Statistics so execute_logical_plan only handles plan execution and emission.domains/simulator/analyzer/src/model.rs (1)
471-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMutate the model instead of rebuilding it field by field.
Lines 473-501 build
temp_model, then destructure every field into a secondSimulatorModelonly to attachresource_group_types. A new field onSimulatorModelmust be added in both places.♻️ Proposed simplification
- let temp_model = SimulatorModel { + let mut model = SimulatorModel { query_engine, arbitrary_resources: resources, tasks, resource_group_types: HashMap::default(), }; - let mut resource_group_types = derive_resource_group_types(&temp_model)?; + let mut resource_group_types = derive_resource_group_types(&model)?; // Bubble up all the used_by_entity fields in the group type decls. for group_type_decl in resource_group_types.values_mut() { for contained_resource_type in &group_type_decl.contains_resource_types { - if let Ok(resource_type) = temp_model + if let Ok(resource_type) = model .arbitrary_resources .resource_type(contained_resource_type) { @@ - Ok(SimulatorModel { - query_engine: temp_model.query_engine, - arbitrary_resources: temp_model.arbitrary_resources, - tasks: temp_model.tasks, - resource_group_types, - }) + model.resource_group_types = resource_group_types; + Ok(model)🤖 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/simulator/analyzer/src/model.rs` around lines 471 - 501, Mutate the existing temp_model instead of constructing a second SimulatorModel: make temp_model mutable, assign the derived and updated resource_group_types to its field after the bubbling loop, then return temp_model directly. Update the construction flow around SimulatorModel and derive_resource_group_types while preserving all existing field values.domains/simulator/analyzer/src/lib.rs (1)
657-684: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant borrow and second index.
Line 663 binds
entrywith&mutbut only readsentry.operator_ids. Line 665 then indexesplain_buildersagain. The same pattern repeats on lines 675-679, 811-813, and 822-826.Read the filter once, then index once.
♻️ Proposed simplification
if let Some(builder_indices) = plain_index.get(&resource_id) { for &builder_idx in builder_indices { - let entry = &mut plain_builders[builder_idx]; - if operator_matches(&entry.operator_ids, task_operator_id) { - plain_builders[builder_idx].builder.try_push(&usage)?; + let entry = &mut plain_builders[builder_idx]; + if operator_matches(&entry.operator_ids, task_operator_id) { + entry.builder.try_push(&usage)?; } } }If the borrow checker rejects the combined borrow, split the filter check first:
let matches = operator_matches(&plain_builders[builder_idx].operator_ids, task_operator_id); if matches { plain_builders[builder_idx].builder.try_push(&usage)?; }🤖 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/simulator/analyzer/src/lib.rs` around lines 657 - 684, In the task usage processing loops, remove the mutable entry borrow and avoid indexing each builder twice: read the operator IDs from the indexed builder, store the match result, then perform a single mutable builder access for try_push. Apply this pattern to both plain and per-state builders here and to the corresponding patterns around the other reported locations.
🤖 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 @.github/workflows/ui.yml:
- Line 177: Update the fallback binding path assigned to base_bindings in the
workflow’s pre-relocation branch to use
base/examples/simulator/server/ts-bindings, while preserving the ui-bindings
path when it exists.
In `@crates/codegen/Cargo.toml`:
- Line 18: Add quent-simulator-instrumentation to the root workspace
dependencies table, then update its dev-dependency declaration in the crate
manifest to use the workspace dependency instead of the local path.
In `@domains/simulator/analyzer/src/lib.rs`:
- Around line 1069-1094: Refactor SimulatorUiAnalyzer::entities_filtered to
validate entity_filter.entity_type_name first, returning the existing
InvalidArgument error for unknown types, then construct a single
tasks.values().filter closure using operator_matches and the time-window span
check. Preserve the current behavior for the recognized "task" type and for
filters without an entity type.
- Around line 1-140: Add accompanying tests across the simulator crates: in
domains/simulator/analyzer/src/lib.rs:1-140, table-test
operator_statistic_quantity, scale_operator_statistic, and
scaled_operator_statistic_name; in
domains/simulator/instrumentation/src/task.rs:73-96, exercise the Task FSM’s
emitted sequence including multi-worker sending and exit; in
domains/simulator/analyzer/src/model.rs:437-502, test
SimulatorModelBuilder::try_push and try_build with valid events and a missing
resource initialization that returns an error; in
domains/simulator/analyzer/src/task.rs:35-96, test active_span with one, two,
and many transitions plus try_to_ui_fsm bytes_per_sec derivation including equal
timestamps; and in domains/simulator/server/src/main.rs:92-99, verify each
exporter name maps to its expected Format and unknown names error.
In `@domains/simulator/analyzer/src/model.rs`:
- Around line 258-265: Replace panic-based invariant handling with
AnalyzerResult propagation at all three sites: in
domains/simulator/analyzer/src/model.rs lines 258-265, update the
TaskBuilder::try_new entry logic to use an Entry match and propagate errors with
?; in domains/simulator/analyzer/src/model.rs lines 447-460, replace
get_mut(&resource_type_name).unwrap() with InvalidTypeName error propagation;
and in domains/simulator/analyzer/src/lib.rs lines 842-865, replace both
panic-producing unwrap_or_else calls with AnalyzerError::BrokenImpl("chunked
bulk: unknown entry id") propagated via ?.
In `@domains/simulator/application/src/main.rs`:
- Around line 701-713: Validate Args::num_threads during clap parsing so only
values of at least one are accepted, preventing an empty self.threads vector. In
the physical_plan.execute task scheduling block, replace the truncated
tasks_per_thread_per_op division with balanced range calculation that assigns
every task, including the remainder, across threads.
In `@domains/simulator/instrumentation/src/task.rs`:
- Around line 84-94: Update the task FSM transitions around JoinPartition’s
producer sequence: include sending in exit_from and add a sending => sending
self-transition so repeated sending events for other workers are accepted before
exit. Preserve the existing sending => queueing transition for the normal
completion path.
In `@domains/simulator/server/src/main.rs`:
- Around line 122-129: Update the importer closure and the
`ImporterFn`/`UiAnalyzer` interfaces so event iterators yield
`ImporterResult<Event<_>>` and propagate errors lazily. Remove the eager
`collect::<Vec<_>>()` buffering in the `Simulator::import_events` path,
preserving errors through the returned iterator without discarding them or
panicking.
In `@domains/simulator/ui-bindings/src/lib.rs`:
- Around line 19-42: Add a Rust test for generate that creates a temporary
output directory containing stale content, invokes generate, verifies generation
succeeds and the stale content is removed, and confirms expected binding files
are emitted in the requested directory. Place the test alongside generate and
use the project’s existing temporary-directory and test conventions.
In `@domains/simulator/ui/Cargo.toml`:
- Around line 1-11: Add the required SPDX copyright and Apache-2.0 license
headers before [package] in domains/simulator/ui/Cargo.toml (lines 1-11) and
domains/simulator/ui-bindings/Cargo.toml (lines 1-11). In both manifests, also
set the new crates to edition 2024 and publish = false as required by the path
instructions.
- Around line 8-11: Move the local quent-analyzer dependency from the
manifest-level path declaration into [workspace.dependencies], then reference it
with workspace = true in domains/simulator/ui/Cargo.toml:8-11. Apply the same
workspace-based declaration to all three local crates in
domains/simulator/ui-bindings/Cargo.toml:8-11, ensuring neither manifest uses
direct path or git dependency declarations.
In `@domains/simulator/ui/src/lib.rs`:
- Around line 26-32: Correct EntityRef::is_resource and
EntityRef::is_resource_group so each returns true only when self matches its
corresponding variant, Resource and ResourceGroup respectively; preserve false
for all other variants. Add unit tests covering matching and non-matching
EntityRef variants for both methods.
---
Other comments:
In `@domains/simulator/server/src/main.rs`:
- Around line 64-68: Update the CORS configuration documentation above the
cors_address argument to reference QUENT_ANALYZER_CORS_ADDRESS consistently with
the env binding and its definition, so the --help text names the effective
environment variable.
---
Nitpick comments:
In `@domains/simulator/analyzer/Cargo.toml`:
- Around line 11-13: Update the path dependencies for
quent-query-engine-analyzer, quent-query-engine-model, and quent-query-engine-ui
to use the direct sibling paths ../../query_engine/analyzer,
../../query_engine/model, and ../../query_engine/ui, matching the existing
relative-path style in Cargo.toml.
In `@domains/simulator/analyzer/src/lib.rs`:
- Around line 657-684: In the task usage processing loops, remove the mutable
entry borrow and avoid indexing each builder twice: read the operator IDs from
the indexed builder, store the match result, then perform a single mutable
builder access for try_push. Apply this pattern to both plain and per-state
builders here and to the corresponding patterns around the other reported
locations.
In `@domains/simulator/analyzer/src/model.rs`:
- Around line 471-501: Mutate the existing temp_model instead of constructing a
second SimulatorModel: make temp_model mutable, assign the derived and updated
resource_group_types to its field after the bubbling loop, then return
temp_model directly. Update the construction flow around SimulatorModel and
derive_resource_group_types while preserving all existing field values.
In `@domains/simulator/application/Cargo.toml`:
- Around line 7-16: Centralize the external dependency versions in the root
workspace: add clap and rand to [workspace.dependencies], then update
domains/simulator/application/Cargo.toml lines 7-16 to use workspace = true for
both; also update domains/simulator/server/Cargo.toml lines 11-12 so axum and
clap use the shared workspace declarations.
In `@domains/simulator/application/src/main.rs`:
- Around line 748-986: Extract the random per-operator attribute construction,
including the attr macro and match on Physical, from execute_logical_plan into a
free function such as operator_statistics(kind: Physical, tasks_processed: u64)
-> Vec<DynamicAttribute>. Keep all existing statistic generation behavior
unchanged, then call this function when populating operator::Statistics so
execute_logical_plan only handles plan execution and emission.
In `@domains/simulator/instrumentation/Cargo.toml`:
- Around line 11-13: Update the quent-query-engine-model dependency in the
instrumentation Cargo.toml manifest to use the direct sibling-domain path
../../query_engine/model instead of traversing through the repository root;
leave the other dependency paths unchanged.
🪄 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: dc4edfb3-1980-4e49-b572-9d3c258e58f3
📒 Files selected for processing (29)
.github/workflows/ui.yml.pre-commit-config.ci.yaml.pre-commit-config.yamlCargo.tomlREADME.mdcrates/codegen/Cargo.tomldocs/domains/query_engine/examples/README.mddocs/domains/query_engine/examples/simulator.mddomains/query_engine/server/Cargo.tomldomains/query_engine/tests/fixed/Cargo.tomldomains/query_engine/tests/fixed/src/lib.rsdomains/simulator/analyzer/Cargo.tomldomains/simulator/analyzer/src/lib.rsdomains/simulator/analyzer/src/model.rsdomains/simulator/analyzer/src/task.rsdomains/simulator/analyzer/src/view.rsdomains/simulator/application/Cargo.tomldomains/simulator/application/src/main.rsdomains/simulator/instrumentation/Cargo.tomldomains/simulator/instrumentation/src/lib.rsdomains/simulator/instrumentation/src/task.rsdomains/simulator/server/Cargo.tomldomains/simulator/server/src/main.rsdomains/simulator/ui-bindings/Cargo.tomldomains/simulator/ui-bindings/src/lib.rsdomains/simulator/ui-bindings/src/main.rsdomains/simulator/ui/Cargo.tomldomains/simulator/ui/src/lib.rsui/REVIEW.md
🛑 Comments failed to post (10)
domains/simulator/analyzer/src/lib.rs (2)
1-140: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
The five new Rust crates ship without tests. The coding guidelines require that new Rust components include accompanying tests. This cohort adds
quent-simulator-instrumentation,quent-simulator,quent-simulator-analyzer,quent-simulator-server, and their UI crates, and none of the reviewed files contains a#[cfg(test)]module or atests/directory. Several behaviors reviewed above are only verifiable by reading the code.
domains/simulator/analyzer/src/lib.rs#L1-L140: add unit tests forscale_operator_statistic,scaled_operator_statistic_name, andoperator_statistic_quantity. These are pure functions with table-driven inputs.domains/simulator/instrumentation/src/task.rs#L73-L96: add a test that drives the Task FSM through the exact sequence the simulator emits, including the multi-workersendingloop and the exit that follows it.domains/simulator/analyzer/src/model.rs#L437-L502: add a test that feeds a small syntheticSimulatorEventstream throughSimulatorModelBuilder::try_pushandtry_build, including a stream that omits a resource initializing event, to lock in error return rather than panic.domains/simulator/analyzer/src/task.rs#L35-L96: add tests foractive_spanwith one, two, and many transitions, and fortry_to_ui_fsmbytes_per_secderivation including the equal-timestamp case.domains/simulator/server/src/main.rs#L92-L99: add a test that asserts each accepted exporter name maps to the expectedFormatand that an unknown name returns an error.As per coding guidelines: "New Rust components must include accompanying tests."
📍 Affects 5 files
domains/simulator/analyzer/src/lib.rs#L1-L140(this comment)domains/simulator/instrumentation/src/task.rs#L73-L96domains/simulator/analyzer/src/model.rs#L437-L502domains/simulator/analyzer/src/task.rs#L35-L96domains/simulator/server/src/main.rs#L92-L99🤖 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/simulator/analyzer/src/lib.rs` around lines 1 - 140, Add accompanying tests across the simulator crates: in domains/simulator/analyzer/src/lib.rs:1-140, table-test operator_statistic_quantity, scale_operator_statistic, and scaled_operator_statistic_name; in domains/simulator/instrumentation/src/task.rs:73-96, exercise the Task FSM’s emitted sequence including multi-worker sending and exit; in domains/simulator/analyzer/src/model.rs:437-502, test SimulatorModelBuilder::try_push and try_build with valid events and a missing resource initialization that returns an error; in domains/simulator/analyzer/src/task.rs:35-96, test active_span with one, two, and many transitions plus try_to_ui_fsm bytes_per_sec derivation including equal timestamps; and in domains/simulator/server/src/main.rs:92-99, verify each exporter name maps to its expected Format and unknown names error.Source: Coding guidelines
1069-1094: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the duplicated task filter closure.
The closure body on lines 1079-1082 and the closure body on lines 1089-1092 are identical. Only the entity-type validation differs.
♻️ Proposed simplification
fn entities_filtered( &self, entity_filter: EntityFilter, operator_ids: HashSet<Uuid>, time_window: SpanNanoSec, ) -> AnalyzerResult<Box<dyn Iterator<Item = &Task> + '_>> { - if let Some(entity_type_name) = entity_filter.entity_type_name { - match entity_type_name.as_str() { - "task" => Ok(Box::new(self.model.tasks.values().filter(move |task| { - operator_matches(&operator_ids, task.operator_id()) - && task.span().is_ok_and(|s| s.intersects(&time_window)) - }))), - _ => Err(AnalyzerError::InvalidArgument(format!( - "{} is not a known entity type in this model", - entity_type_name - ))), - } - } else { - Ok(Box::new(self.model.tasks.values().filter(move |task| { - operator_matches(&operator_ids, task.operator_id()) - && task.span().is_ok_and(|s| s.intersects(&time_window)) - }))) - } + if let Some(entity_type_name) = &entity_filter.entity_type_name + && entity_type_name != "task" + { + return Err(AnalyzerError::InvalidArgument(format!( + "{entity_type_name} is not a known entity type in this model" + ))); + } + Ok(Box::new(self.model.tasks.values().filter(move |task| { + operator_matches(&operator_ids, task.operator_id()) + && task.span().is_ok_and(|s| s.intersects(&time_window)) + }))) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.impl SimulatorUiAnalyzer { /// Return an iterator over all tasks, filtered by time window and operator ids. fn entities_filtered( &self, entity_filter: EntityFilter, operator_ids: HashSet<Uuid>, time_window: SpanNanoSec, ) -> AnalyzerResult<Box<dyn Iterator<Item = &Task> + '_>> { if let Some(entity_type_name) = &entity_filter.entity_type_name && entity_type_name != "task" { return Err(AnalyzerError::InvalidArgument(format!( "{entity_type_name} is not a known entity type in this model" ))); } Ok(Box::new(self.model.tasks.values().filter(move |task| { operator_matches(&operator_ids, task.operator_id()) && task.span().is_ok_and(|s| s.intersects(&time_window)) }))) }🤖 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/simulator/analyzer/src/lib.rs` around lines 1069 - 1094, Refactor SimulatorUiAnalyzer::entities_filtered to validate entity_filter.entity_type_name first, returning the existing InvalidArgument error for unknown types, then construct a single tasks.values().filter closure using operator_matches and the time-window span check. Preserve the current behavior for the recognized "task" type and for filters without an entity type.domains/simulator/analyzer/src/model.rs (1)
258-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Invariant violations abort the process instead of returning
AnalyzerError. Four sites in the analyzer crate panic on conditions the authors believe are unreachable. Every one of these sites sits on a path thatanalyzer_service_routerindomains/simulator/server/src/main.rsruns while serving HTTP requests, and every enclosing function already returnsAnalyzerResult. A malformed, truncated, or out-of-order event stream then crashes the service instead of producing an error response.domains/simulator/analyzer/src/lib.rsline 873 already models the correct behavior withAnalyzerError::BrokenImpl.
domains/simulator/analyzer/src/model.rs#L258-L265: replaceTaskBuilder::try_new(id).unwrap()with anEntrymatch that propagates the error using?.domains/simulator/analyzer/src/model.rs#L447-L460: replace theget_mut(&resource_type_name).unwrap()with.ok_or_else(|| AnalyzerError::InvalidTypeName(resource_type_name.clone()))?.domains/simulator/analyzer/src/lib.rs#L842-L865: replace bothunwrap_or_else(|| panic!(...))calls with.ok_or(AnalyzerError::BrokenImpl("chunked bulk: unknown entry id"))?.📍 Affects 2 files
domains/simulator/analyzer/src/model.rs#L258-L265(this comment)domains/simulator/analyzer/src/model.rs#L447-L460domains/simulator/analyzer/src/lib.rs#L842-L865🤖 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/simulator/analyzer/src/model.rs` around lines 258 - 265, Replace panic-based invariant handling with AnalyzerResult propagation at all three sites: in domains/simulator/analyzer/src/model.rs lines 258-265, update the TaskBuilder::try_new entry logic to use an Entry match and propagate errors with ?; in domains/simulator/analyzer/src/model.rs lines 447-460, replace get_mut(&resource_type_name).unwrap() with InvalidTypeName error propagation; and in domains/simulator/analyzer/src/lib.rs lines 842-865, replace both panic-producing unwrap_or_else calls with AnalyzerError::BrokenImpl("chunked bulk: unknown entry id") propagated via ?.domains/simulator/application/src/main.rs (1)
701-713: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
--num-threads 0causes a division-by-zero panic.Line 703 divides by
self.threads.len().Args::num_threadsis ausizewith no lower bound, so--num-threads 0produces an emptythreadsvector and panics at this line.The same line also truncates silently. With
--num-tasks 32 --num-threads 5, each thread runs 6 tasks and the simulator emits 30 tasks per operator instead of 32.Validate
num_threadsat parse time with a clap value parser, and distribute the remainder.🐛 Proposed guard on the argument
/// Number of threads per worker thread pool - #[arg(long, default_value_t = 2)] + #[arg(long, default_value_t = 2, value_parser = clap::value_parser!(u16).range(1..))] num_threads: usize,Adjust the field type or convert the parsed value as needed. Alternatively guard at the use site:
- let tasks_per_thread_per_op = num_tasks / self.threads.len(); + if self.threads.is_empty() { + return; + } + let tasks_per_thread_per_op = num_tasks / self.threads.len();🤖 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/simulator/application/src/main.rs` around lines 701 - 713, Validate Args::num_threads during clap parsing so only values of at least one are accepted, preventing an empty self.threads vector. In the physical_plan.execute task scheduling block, replace the truncated tasks_per_thread_per_op division with balanced range calculation that assigns every task, including the remainder, across threads.domains/simulator/instrumentation/src/task.rs (1)
84-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect the fsm! macro contract and the simulator send loop. set -euo pipefail # Locate the fsm! macro implementation. fd -t f -e rs . crates/model | xargs rg -n -C 10 'exit_from|macro_rules!\s+fsm|fn\s+fsm\b' || true # Show how transitions and exit states are validated/generated. rg -n -C 15 'exit_from' --type=rust crates || true # Confirm the repeated sending calls in the producer. rg -n -C 12 'fn execute_physical_operator_task' domains/simulator/application/src/main.rsRepository: rapidsai/quent
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- FSM declaration ---' cat -n domains/simulator/instrumentation/src/task.rs | sed -n '70,105p' printf '%s\n' '--- Producer send loop ---' cat -n domains/simulator/application/src/main.rs | sed -n '600,665p' printf '%s\n' '--- Generated transition and exit behavior ---' cat -n crates/model-macros/src/fsm_macro.rs | sed -n '243,360p' rg -n -C 8 'TransitionEndpoint|transition\(|exit\(|Exit' crates/model crates/model-macros domains/simulator --type rust | head -n 260Repository: rapidsai/quent
Length of output: 26110
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- FSM-generated handle ---' cat -n crates/model-macros/src/fsm_macro.rs | sed -n '360,490p' printf '%s\n' '--- State-generated handle lifecycle methods ---' cat -n crates/model-macros/src/state_macro.rs | sed -n '300,390p' printf '%s\n' '--- Runtime transition emission and exit guards ---' rg -n -C 12 'struct .*Handle|fn emit_transition|fn exit|exited|TransitionInfo' crates --type rust | head -n 320Repository: rapidsai/quent
Length of output: 31074
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- FSM handle implementation ---' cat -n crates/model-macros/src/fsm_macro.rs | sed -n '483,570p' printf '%s\n' '--- All emit_transition definitions ---' rg -n -C 15 'emit_transition' crates --type rust printf '%s\n' '--- Exit call sites and generated exit method ---' rg -n -C 12 '\.exit\(\)|fn exit' domains crates --type rust | head -n 220Repository: rapidsai/quent
Length of output: 31078
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- JoinPartition send selection ---' rg -n -C 18 'JoinPartition|let send|send:' domains/simulator --type rust printf '%s\n' '--- Consumers of FSM transition metadata ---' rg -n -C 12 'fsm_def\.transitions|FsmDef|TransitionDef|TransitionEndpoint' crates domains --type rust | head -n 360 printf '%s\n' '--- Analyzer FSM validation and transition handling ---' rg -n -C 14 'unreachable|invalid.*transition|transition.*valid|state.*transition|FsmTypeDecl|FsmTransitionDecl' crates/analyzer crates/yaml crates --type rust | head -n 360Repository: rapidsai/quent
Length of output: 50370
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- Fsm transition declaration types ---' rg -n -C 18 'enum FsmTransitionDecl|struct FsmTypeDecl|trait FsmTypeDeclaration|transitions\(' crates/analyzer/src/fsm crates/analyzer/src --type rust printf '%s\n' '--- Uses of FsmTypeDeclaration transitions ---' rg -n -C 16 'fsm_type_declaration|FsmTransitionDecl|FsmTypeDecl' crates/analyzer/src crates --type rust | head -n 300Repository: rapidsai/quent
Length of output: 42285
Align the task FSM with the producer sequence.
JoinPartitionemitssendingonce per other worker, then emitsexit. Addsending => sendingandsendingtoexit_from, or change the producer sequence.🤖 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/simulator/instrumentation/src/task.rs` around lines 84 - 94, Update the task FSM transitions around JoinPartition’s producer sequence: include sending in exit_from and add a sending => sending self-transition so repeated sending events for other workers are accepted before exit. Preserve the existing sending => queueing transition for the normal completion path.domains/simulator/server/src/main.rs (1)
122-129: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Determine the importer closure signature expected by analyzer_service_router. set -euo pipefail rg -n -C 15 'fn analyzer_service_router' --type=rust domains/query_engine/server/src # Importer type alias and its item type. rg -n -C 8 'Importer|Box<dyn Iterator' --type=rust domains/query_engine/server/src || trueRepository: rapidsai/quent
Length of output: 17102
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- simulator importer and call site ---' cat -n domains/simulator/server/src/main.rs | sed -n '105,140p' rg -n -C 12 'fn import_events|import_events|ImporterResult|type ImporterResult|struct Importer' --type rust . printf '%s\n' '--- analyzer cache consumers ---' cat -n domains/query_engine/server/src/analyzer_cache.rs | sed -n '145,260p' rg -n -C 8 'chain_context_events|for .*event|Analyzer::|analyzers' domains/query_engine/server/src/analyzer_cache.rsRepository: rapidsai/quent
Length of output: 50370
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re files = { "importer_fn": Path("domains/query_engine/server/src/analyzer_cache.rs"), "simulator_main": Path("domains/simulator/server/src/main.rs"), "ui_analyzer": Path("domains/query_engine/analyzer/src/ui.rs"), "simulator_analyzer": Path("domains/simulator/analyzer/src/lib.rs"), } text = {name: path.read_text() for name, path in files.items()} checks = { "ImporterFn consumes infallible events": re.search(r"pub type ImporterFn.*Iterator<Item = Event<", text["importer_fn"], re.S), "chain_context_events consumes infallible events": re.search(r"chain_context_events.*?Iterator<Item = Event<", text["importer_fn"], re.S), "Simulator importer yields fallible events": re.search(r"Item\s*=\s*quent_model::io::ImporterResult<\s*quent_model::Event<", Path("crates/model-macros/src/model_macro.rs").read_text(), re.S), "Simulator main collects importer results": "collect::<quent_io::ImporterResult<Vec<_>>>()?" in text["simulator_main"], "UiAnalyzer constructors consume infallible events": bool(re.search(r"Iterator<Item = Event<", text["ui_analyzer"])), } for name, passed in checks.items(): print(f"{'PASS' if passed else 'FAIL'}: {name}") PY printf '%s\n' '--- analyzer construction signatures ---' rg -n -C 6 'fn try_new|fn extract_engine|Iterator<Item = Event|ImporterResult' \ domains/query_engine/analyzer domains/simulator/analyzerRepository: rapidsai/quent
Length of output: 22988
Accept fallible event streams before removing the buffer
Simulator::import_eventsyieldsImporterResult<Event<_>>, whileImporterFnandUiAnalyzerrequireIterator<Item = Event<_>>. Change these interfaces to propagate importer errors lazily before removing theVec; do not discard errors or panic.🤖 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/simulator/server/src/main.rs` around lines 122 - 129, Update the importer closure and the `ImporterFn`/`UiAnalyzer` interfaces so event iterators yield `ImporterResult<Event<_>>` and propagate errors lazily. Remove the eager `collect::<Vec<_>>()` buffering in the `Simulator::import_events` path, preserving errors through the returned iterator without discarding them or panicking.domains/simulator/ui-bindings/src/lib.rs (1)
19-42: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a test for binding generation.
Test that
generateremoves stale output, completes generation, and emits bindings into the requested directory. This protects the generated UI contract and the output-directory behavior. As per coding guidelines, “New Rust components must include accompanying tests.”🤖 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/simulator/ui-bindings/src/lib.rs` around lines 19 - 42, Add a Rust test for generate that creates a temporary output directory containing stale content, invokes generate, verifies generation succeeds and the stale content is removed, and confirms expected binding files are emitted in the requested directory. Place the test alongside generate and use the project’s existing temporary-directory and test conventions.Source: Coding guidelines
domains/simulator/ui/Cargo.toml (2)
1-11: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add SPDX headers to both new crate manifests.
domains/simulator/ui/Cargo.toml#L1-L11: add the required SPDX copyright and Apache-2.0 license header before[package].domains/simulator/ui-bindings/Cargo.toml#L1-L11: add the required SPDX copyright and Apache-2.0 license header before[package].As per path instructions, “New crates: edition 2024, publish = false, SPDX headers.”
📍 Affects 2 files
domains/simulator/ui/Cargo.toml#L1-L11(this comment)domains/simulator/ui-bindings/Cargo.toml#L1-L11🤖 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/simulator/ui/Cargo.toml` around lines 1 - 11, Add the required SPDX copyright and Apache-2.0 license headers before [package] in domains/simulator/ui/Cargo.toml (lines 1-11) and domains/simulator/ui-bindings/Cargo.toml (lines 1-11). In both manifests, also set the new crates to edition 2024 and publish = false as required by the path instructions.Source: Path instructions
8-11: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use workspace dependency declarations for local crates.
domains/simulator/ui/Cargo.toml#L8-L11: declarequent-analyzerin[workspace.dependencies]and usequent-analyzer.workspace = true.domains/simulator/ui-bindings/Cargo.toml#L8-L11: declare the three local crates in[workspace.dependencies]and useworkspace = truein this manifest.As per path instructions, “Dependencies come from [workspace.dependencies] via
workspace = true; no git deps.”📍 Affects 2 files
domains/simulator/ui/Cargo.toml#L8-L11(this comment)domains/simulator/ui-bindings/Cargo.toml#L8-L11🤖 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/simulator/ui/Cargo.toml` around lines 8 - 11, Move the local quent-analyzer dependency from the manifest-level path declaration into [workspace.dependencies], then reference it with workspace = true in domains/simulator/ui/Cargo.toml:8-11. Apply the same workspace-based declaration to all three local crates in domains/simulator/ui-bindings/Cargo.toml:8-11, ensuring neither manifest uses direct path or git dependency declarations.Source: Path instructions
domains/simulator/ui/src/lib.rs (1)
26-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return
trueonly for the matching entity type.Line 28 reports
falseforEntityRef::Resourceandtruefor every non-resource variant. Line 31 reportsfalseforEntityRef::Taskinstead of checkingEntityRef::ResourceGroup. This misclassifies entities in analyzer resource views.Proposed fix
fn is_resource(&self) -> bool { - !matches!(self, EntityRef::Resource(_)) + matches!(self, EntityRef::Resource(_)) } fn is_resource_group(&self) -> bool { - !matches!(self, EntityRef::Task(_)) + matches!(self, EntityRef::ResourceGroup(_)) }Add unit tests for both matching and non-matching variants. As per coding guidelines, “New Rust components must include accompanying tests.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.impl EntityId for EntityRef { fn is_resource(&self) -> bool { matches!(self, EntityRef::Resource(_)) } fn is_resource_group(&self) -> bool { matches!(self, EntityRef::ResourceGroup(_)) }🤖 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/simulator/ui/src/lib.rs` around lines 26 - 32, Correct EntityRef::is_resource and EntityRef::is_resource_group so each returns true only when self matches its corresponding variant, Resource and ResourceGroup respectively; preserve false for all other variants. Add unit tests covering matching and non-matching EntityRef variants for both methods.Source: Coding guidelines
Contributor
|
Looks like there are some conflicts that need to be fixed first. |
johanpel
marked this pull request as draft
August 27, 2026 07:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #102
The simulator is a query engine simulator rather than an example, so it now lives under
domains/simulator/alongsidedomains/query_engine/. Updated workspace members and default members in the rootCargo.toml, Cargo path dependencies (crates/codegen,domains/query_engine/server,domains/query_engine/tests/fixed), README/docs references, pre-commit exclude patterns, and CI base-branch paths in.github/workflows/ui.yml.