container pull/dry-run/custom benchmark#141
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive feature set: custom benchmarks with templatable commands, container image pull workflow via SLURM or local import, sidecar process orchestration on the head node, custom frontend support, enhanced dry-run display showing execution plans, and backward-compatible container entry schema supporting both string paths and new path+source dictionaries for container-pull integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant SrcCtl as srtctl
participant Enroot as Enroot
participant SLURM as SLURM/sbatch
participant LoginNode as Login Node
User->>SrcCtl: container_pull(force, local)
SrcCtl->>SrcCtl: Retrieve containers from config
SrcCtl->>SrcCtl: Display status table
alt local=True
SrcCtl->>LoginNode: Process each container
LoginNode->>Enroot: enroot import <source>
Enroot-->>LoginNode: Download complete
LoginNode-->>SrcCtl: Update status
else local=False (batch)
SrcCtl->>SrcCtl: Build sbatch script
SrcCtl->>SLURM: sbatch submit_script
SLURM-->>SrcCtl: Job ID
SLURM->>LoginNode: Execute job script
LoginNode->>Enroot: enroot import <source>
Enroot-->>LoginNode: Download complete
end
SrcCtl-->>User: Return success/error status
sequenceDiagram
participant Orchestrator as SweepOrchestrator
participant Workers as WorkerStage
participant Sidecars as SidecarStageMixin
participant Frontend as FrontendStageMixin
participant HeadNode as Head Node Process
Orchestrator->>Workers: start_workers()
Workers-->>Orchestrator: Workers started
Orchestrator->>Orchestrator: Check config.sidecars
alt config.sidecars present
Orchestrator->>Sidecars: start_sidecars()
loop for each sidecar
Sidecars->>Sidecars: Build environment (ETCD, NATS, DYN_REQUEST_PLANE)
Sidecars->>Sidecars: Build bash preamble
Sidecars->>HeadNode: start_srun_process(sidecar_command)
HeadNode-->>Sidecars: ManagedProcess
end
Sidecars-->>Orchestrator: Sidecars running
end
Orchestrator->>Frontend: start_frontend()
Frontend-->>Orchestrator: Frontend ready
Orchestrator->>Orchestrator: Benchmark stage
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/srtctl/cli/mixins/benchmark_stage.py (1)
45-53:⚠️ Potential issue | 🟡 MinorPipeline failures: Mixin property stubs need proper abstract implementation.
The type checker (
ty) reports errors because these property stubs implicitly returnNoneinstead of the declared return types. While this is a pre-existing pattern (not introduced by this PR), it should be addressed:🔧 Suggested fix
`@property` def endpoints(self) -> list["Endpoint"]: """Endpoint allocation topology.""" - ... + raise NotImplementedError("Subclass must implement endpoints property") `@property` def backend_processes(self) -> list["Process"]: """Backend worker processes.""" - ... + raise NotImplementedError("Subclass must implement backend_processes property")Alternatively, if this mixin is always used with ABC:
from abc import abstractmethod `@property` `@abstractmethod` def endpoints(self) -> list["Endpoint"]: """Endpoint allocation topology."""
🤖 Fix all issues with AI agents
In `@docs/cli.md`:
- Around line 396-408: The fenced code block showing the ASCII container table
is missing a language specifier (MD040); update the opening fence for that block
(the triple-backtick that begins the ASCII table) to include a language token
such as "text" so the block becomes ```text, ensuring the entire ASCII table and
following lines remain inside that fenced block; no other content changes are
needed.
- Around line 302-334: The fenced code block showing CLI output (the block
beginning with "Runtime Context" and the example tables) lacks a language
specifier; update that opening triple-backtick to include a language tag (e.g.,
```text) so linters (MD040) recognize it as plain text output and the block
renders appropriately.
In `@src/srtctl/benchmarks/custom.py`:
- Around line 95-101: The build_command function currently returns an argv list
from shlex.split(cmd_str), which prevents shell operators (|, &&, >, <, etc.)
from being interpreted; update build_command to scan cmd_str for shell
metacharacters (e.g., pipes, redirects, &&, ||, ;, $, ``, *, ?, [ ] ) and if any
are present return ["bash", "-c", cmd_str] instead of shlex.split, otherwise
keep using shlex.split(cmd_str) with the existing ValueError fallback; reference
build_command, cmd_str, and shlex.split when making the change (or alternatively
add a clear docstring on build_command noting that shell operators are not
supported if you prefer not to implement the detection).
In `@src/srtctl/cli/container_pull.py`:
- Line 125: The mkdir command uses dirname with an unquoted {path} which breaks
when the path contains spaces or special characters; update the call where
script_lines.append is building the shell line (the script_lines.append(f'mkdir
-p "$(dirname {path})"') expression) to quote the path inside the command
substitution so dirname receives a single argument (e.g. ensure dirname is
invoked as dirname "…{path}…" by adding proper double quotes around {path}
inside the $(...) expansion).
In `@src/srtctl/cli/submit.py`:
- Around line 174-179: The dry-run output currently prints config.environment
values verbatim (see config.environment usage in submit.py and the console.print
loop), which can leak secrets; change the loop that prints environment variables
to redact common secret keys (e.g., keys containing "KEY", "SECRET", "TOKEN",
"PASSWORD", "API", "ACCESS") by replacing their values with a masked placeholder
like "<redacted>" and add an explicit opt-in flag (e.g., --show-secrets /
show_secrets) to bypass redaction when the user deliberately requests full
values; update the printing logic in the submit/dry-run path to consult that
flag and the redaction list so secrets are masked by default.
In `@src/srtctl/core/config.py`:
- Around line 223-229: The code currently uses an empty string when a dict entry
lacks "path" which can silently fail; in the loop over containers.items()
(variables: containers, name, entry, result) change the dict branch to validate
that "path" exists and is non-empty and if not raise a clear ValueError
(including the container name) instead of defaulting to "". Populate
result[name] = {"path": path, "source": entry.get("source")} only after
validation so downstream code never receives an empty path.
In `@src/srtctl/core/runtime.py`:
- Around line 354-360: The dry-run currently builds container_mounts only with
log_dir and model_path, which omits mounts from default_mounts, extra_mount, and
config.container_mounts; update the dry-run assembly in runtime.py to mirror the
logic used by from_config: start with default_mounts, then apply
config.container_mounts and extra_mount (respecting keys/targets), and finally
add the log_dir and model_path entries, but skip any filesystem existence
checks—ensure you update the container_mounts dict construction and reuse the
same mount key/Path normalization used by from_config to keep behavior
consistent.
- Around line 342-344: The dry-run log_dir is created using a relative
Path("./dry-run-outputs") which violates RuntimeContext’s contract that all
paths be absolute; update the code that sets log_dir (the variable in the
runtime logic that uses job_id) to produce an absolute path (e.g., resolve or
join against an absolute base) so log_dir is absolute before further use and
continue to include the job_id/"logs" suffix; ensure the change is made where
log_dir is defined so any other uses of log_dir or RuntimeContext observe an
absolute path.
- Around line 80-109: The from_mock classmethod currently constructs nodelist
without checking num_nodes and will IndexError when num_nodes==0; add an
explicit validation at the start of from_mock (before building nodelist) to
raise ValueError("num_nodes must be >= 1") when num_nodes < 1 so callers get a
clear error; keep the existing etcd_nats_dedicated_node check for num_nodes < 2
as-is and reference the same symbols: from_mock, nodelist,
etcd_nats_dedicated_node, head, infra, worker.
In `@src/srtctl/core/schema.py`:
- Around line 195-233: In ContainersField._deserialize, add explicit type
validation for path and source to avoid accepting non-string values: when
handling the string entry case in ContainersField, ensure the entry is a str
before creating ContainerEntry(path=...), and when handling the dict case (in
ContainersField._deserialize for entry dict), validate that entry["path"] exists
and is a str and that entry.get("source") is either None or a str; if these
checks fail, raise a ValidationError with a clear message referencing the
container name. Keep using ContainerEntry(path=..., source=...) once types are
confirmed and update the ValidationError messages to indicate the expected
types.
🧹 Nitpick comments (4)
src/srtctl/core/config.py (1)
132-141: Consider extracting duplicated container entry resolution logic.The same pattern for handling both string and dict container formats appears here and again in
resolve_container_path(). Extracting a helper would improve maintainability:♻️ Suggested refactor
def _extract_container_path(entry: str | dict[str, Any]) -> str | None: """Extract path from a container entry (string or dict format).""" if isinstance(entry, str): return entry elif isinstance(entry, dict) and "path" in entry: return entry["path"] return NoneThen use it in all four locations:
- if isinstance(container_entry, str): - resolved_container = container_entry - elif isinstance(container_entry, dict) and "path" in container_entry: - resolved_container = container_entry["path"] - else: - resolved_container = container_entry + resolved_container = _extract_container_path(container_entry) + if resolved_container is None: + resolved_container = container_entry # fallback for unknown formatsAlso applies to: 153-163
src/srtctl/core/schema.py (1)
256-275: ClusterConfig should be frozen like other config objects.
This class remains mutable, which diverges from the project’s config immutability rule. Consider@dataclass(frozen=True)for consistency.
As per coding guidelines: Use frozen dataclasses for all configuration objects with@dataclass(frozen=True).src/srtctl/cli/container_pull.py (2)
60-60: Consider usingTypedDictfor container entry structure.Per coding guidelines,
TypedDictis preferred for typing dicts from external sources like YAML. This would provide better type safety and IDE support.♻️ Suggested TypedDict definition
from typing import TypedDict class ContainerEntry(TypedDict, total=False): path: str source: str # OptionalThen update the signature:
-def submit_container_pull_job(containers: dict[str, dict], force: bool = False) -> str | None: +def submit_container_pull_job(containers: dict[str, ContainerEntry], force: bool = False) -> str | None:
136-147: Temp file may leak ifsubprocess.runraises an unexpected exception.If
subprocess.runraises an exception other thanFileNotFoundError(e.g.,OSError,PermissionError), the temp file won't be cleaned up. Consider usingtry/finallyto ensure cleanup.♻️ Proposed refactor
try: with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) as f: f.write(script_content) script_path = f.name - result = subprocess.run( - ["sbatch", script_path], - capture_output=True, - text=True, - ) - - # Clean up temp file - Path(script_path).unlink() + try: + result = subprocess.run( + ["sbatch", script_path], + capture_output=True, + text=True, + ) + finally: + Path(script_path).unlink() if result.returncode != 0:
| ``` | ||
| Runtime Context | ||
| Job ID: DRY_RUN | ||
| Run Name: my-config_DRY_RUN | ||
| Head Node: node-01 | ||
| Worker Nodes: node-01, node-02, node-03 | ||
| Container: /path/to/container.sqsh | ||
| Model: /path/to/model | ||
|
|
||
| Endpoint Allocation | ||
| ┌──────────────┬─────────┬──────────────────────┬─────────┬────────────┐ | ||
| │ Endpoint │ Mode │ Nodes │ GPUs │ Total GPUs │ | ||
| ├──────────────┼─────────┼──────────────────────┼─────────┼────────────┤ | ||
| │ prefill_0 │ prefill │ node-01 │ [0-7] │ 8 │ | ||
| │ decode_0 │ decode │ node-02 │ [0-7] │ 8 │ | ||
| │ decode_1 │ decode │ node-03 │ [0-7] │ 8 │ | ||
| └──────────────┴─────────┴──────────────────────┴─────────┴────────────┘ | ||
|
|
||
| Process Commands | ||
| [prefill_0] | ||
| Node: node-01 | ||
| GPUs: 0,1,2,3,4,5,6,7 | ||
| HTTP Port: 30000 | ||
| Command: srun --nodes=1 --nodelist=node-01 ... | ||
|
|
||
| Container Mounts | ||
| ┌────────────────────────┬────────────────┐ | ||
| │ Host Path │ Container Path │ | ||
| ├────────────────────────┼────────────────┤ | ||
| │ /path/to/logs │ /logs │ | ||
| │ /path/to/model │ /model │ | ||
| └────────────────────────┴────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
Static analysis flagged this code block as missing a language specifier (MD040). Since this shows CLI output, use text or leave it unspecified with triple backticks only (which some linters accept).
📝 Suggested fix
-```
+```text
Runtime Context
Job ID: DRY_RUN📝 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.
| ``` | |
| Runtime Context | |
| Job ID: DRY_RUN | |
| Run Name: my-config_DRY_RUN | |
| Head Node: node-01 | |
| Worker Nodes: node-01, node-02, node-03 | |
| Container: /path/to/container.sqsh | |
| Model: /path/to/model | |
| Endpoint Allocation | |
| ┌──────────────┬─────────┬──────────────────────┬─────────┬────────────┐ | |
| │ Endpoint │ Mode │ Nodes │ GPUs │ Total GPUs │ | |
| ├──────────────┼─────────┼──────────────────────┼─────────┼────────────┤ | |
| │ prefill_0 │ prefill │ node-01 │ [0-7] │ 8 │ | |
| │ decode_0 │ decode │ node-02 │ [0-7] │ 8 │ | |
| │ decode_1 │ decode │ node-03 │ [0-7] │ 8 │ | |
| └──────────────┴─────────┴──────────────────────┴─────────┴────────────┘ | |
| Process Commands | |
| [prefill_0] | |
| Node: node-01 | |
| GPUs: 0,1,2,3,4,5,6,7 | |
| HTTP Port: 30000 | |
| Command: srun --nodes=1 --nodelist=node-01 ... | |
| Container Mounts | |
| ┌────────────────────────┬────────────────┐ | |
| │ Host Path │ Container Path │ | |
| ├────────────────────────┼────────────────┤ | |
| │ /path/to/logs │ /logs │ | |
| │ /path/to/model │ /model │ | |
| └────────────────────────┴────────────────┘ | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/cli.md` around lines 302 - 334, The fenced code block showing CLI output
(the block beginning with "Runtime Context" and the example tables) lacks a
language specifier; update that opening triple-backtick to include a language
tag (e.g., ```text) so linters (MD040) recognize it as plain text output and the
block renders appropriately.
| ``` | ||
| ┌─ Containers ────────────────────────────────────────────────────────┐ | ||
| │ Name │ Path │ Source │ Status │ | ||
| ├────────┼────────────────────────────────┼─────────────────────┼──────────────┤ | ||
| │ nginx │ /shared/containers/nginx.sqsh │ none │ no source │ | ||
| │ sglang │ /shared/containers/sglang.sqsh │ docker://nvcr.io/.. │ will download│ | ||
| └────────┴────────────────────────────────┴─────────────────────┴──────────────┘ | ||
|
|
||
| sglang: downloading from docker://nvcr.io/nvidia/sglang:0.4.1... | ||
| sglang: saved to /shared/containers/sglang.sqsh | ||
|
|
||
| Summary: 1 downloaded, 1 skipped | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
Static analysis flagged this code block as missing a language specifier (MD040).
📝 Suggested fix
-```
+```text
┌─ Containers ────────────────────────────────────────────────────────┐📝 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.
| ``` | |
| ┌─ Containers ────────────────────────────────────────────────────────┐ | |
| │ Name │ Path │ Source │ Status │ | |
| ├────────┼────────────────────────────────┼─────────────────────┼──────────────┤ | |
| │ nginx │ /shared/containers/nginx.sqsh │ none │ no source │ | |
| │ sglang │ /shared/containers/sglang.sqsh │ docker://nvcr.io/.. │ will download│ | |
| └────────┴────────────────────────────────┴─────────────────────┴──────────────┘ | |
| sglang: downloading from docker://nvcr.io/nvidia/sglang:0.4.1... | |
| sglang: saved to /shared/containers/sglang.sqsh | |
| Summary: 1 downloaded, 1 skipped | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 396-396: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/cli.md` around lines 396 - 408, The fenced code block showing the ASCII
container table is missing a language specifier (MD040); update the opening
fence for that block (the triple-backtick that begins the ASCII table) to
include a language token such as "text" so the block becomes ```text, ensuring
the entire ASCII table and following lines remain inside that fenced block; no
other content changes are needed.
|
|
||
| for name, source, path in downloads: | ||
| # Ensure parent directory exists | ||
| script_lines.append(f'mkdir -p "$(dirname {path})"') |
There was a problem hiding this comment.
Missing quotes around path in dirname command substitution.
If the path contains spaces or special characters, dirname will receive incorrect arguments because {path} is not quoted inside $(...). This could cause the script to fail or create directories in unexpected locations.
🐛 Proposed fix
- script_lines.append(f'mkdir -p "$(dirname {path})"')
+ script_lines.append(f'mkdir -p "$(dirname "{path}")"')📝 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.
| script_lines.append(f'mkdir -p "$(dirname {path})"') | |
| script_lines.append(f'mkdir -p "$(dirname "{path}")"') |
🤖 Prompt for AI Agents
In `@src/srtctl/cli/container_pull.py` at line 125, The mkdir command uses dirname
with an unquoted {path} which breaks when the path contains spaces or special
characters; update the call where script_lines.append is building the shell line
(the script_lines.append(f'mkdir -p "$(dirname {path})"') expression) to quote
the path inside the command substitution so dirname receives a single argument
(e.g. ensure dirname is invoked as dirname "…{path}…" by adding proper double
quotes around {path} inside the $(...) expansion).
| # Environment Variables section | ||
| if config.environment: | ||
| console.print("[bold cyan]Global Environment Variables[/]") | ||
| for key, value in config.environment.items(): | ||
| console.print(f" {key}={value}") | ||
| console.print() |
There was a problem hiding this comment.
Redact secrets in dry‑run environment output.
The enhanced dry-run prints config.environment values verbatim; any API keys/tokens will leak to terminals/CI logs. Please mask common secret keys or gate the output behind an explicit flag.
🔒 Suggested redaction for sensitive env vars
@@
- if config.environment:
- console.print("[bold cyan]Global Environment Variables[/]")
- for key, value in config.environment.items():
- console.print(f" {key}={value}")
+ if config.environment:
+ console.print("[bold cyan]Global Environment Variables[/]")
+ sensitive_markers = ("TOKEN", "SECRET", "PASSWORD", "KEY")
+ for key, value in config.environment.items():
+ masked = "******" if any(m in key.upper() for m in sensitive_markers) else value
+ console.print(f" {key}={masked}")🤖 Prompt for AI Agents
In `@src/srtctl/cli/submit.py` around lines 174 - 179, The dry-run output
currently prints config.environment values verbatim (see config.environment
usage in submit.py and the console.print loop), which can leak secrets; change
the loop that prints environment variables to redact common secret keys (e.g.,
keys containing "KEY", "SECRET", "TOKEN", "PASSWORD", "API", "ACCESS") by
replacing their values with a masked placeholder like "<redacted>" and add an
explicit opt-in flag (e.g., --show-secrets / show_secrets) to bypass redaction
when the user deliberately requests full values; update the printing logic in
the submit/dry-run path to consult that flag and the redaction list so secrets
are masked by default.
| result: dict[str, dict[str, Any]] = {} | ||
| for name, entry in containers.items(): | ||
| if isinstance(entry, str): | ||
| result[name] = {"path": entry, "source": None} | ||
| elif isinstance(entry, dict): | ||
| result[name] = {"path": entry.get("path", ""), "source": entry.get("source")} | ||
| return result |
There was a problem hiding this comment.
Empty string default for missing path may cause silent failures.
When a dict entry lacks a path key, this defaults to an empty string. This could lead to confusing errors downstream when the empty path is used.
🛡️ Suggested improvement
for name, entry in containers.items():
if isinstance(entry, str):
result[name] = {"path": entry, "source": None}
elif isinstance(entry, dict):
- result[name] = {"path": entry.get("path", ""), "source": entry.get("source")}
+ path = entry.get("path")
+ if not path:
+ logger.warning(f"Container '{name}' has dict format but missing 'path' key")
+ continue
+ result[name] = {"path": path, "source": entry.get("source")}
return result🤖 Prompt for AI Agents
In `@src/srtctl/core/config.py` around lines 223 - 229, The code currently uses an
empty string when a dict entry lacks "path" which can silently fail; in the loop
over containers.items() (variables: containers, name, entry, result) change the
dict branch to validate that "path" exists and is non-empty and if not raise a
clear ValueError (including the container name) instead of defaulting to "".
Populate result[name] = {"path": path, "source": entry.get("source")} only after
validation so downstream code never receives an empty path.
| @classmethod | ||
| def from_mock( | ||
| cls, | ||
| num_nodes: int, | ||
| etcd_nats_dedicated_node: bool = False, | ||
| ) -> "Nodes": | ||
| """Create mock Nodes for dry-run mode. | ||
|
|
||
| Generates node names like node-01, node-02, etc. | ||
|
|
||
| Args: | ||
| num_nodes: Total number of nodes to simulate | ||
| etcd_nats_dedicated_node: If True, dedicate first node for etcd/nats | ||
| """ | ||
| nodelist = [f"node-{i:02d}" for i in range(1, num_nodes + 1)] | ||
|
|
||
| if etcd_nats_dedicated_node: | ||
| if num_nodes < 2: | ||
| raise ValueError("etcd_nats_dedicated_node requires at least 2 nodes") | ||
| infra = nodelist[0] | ||
| head = nodelist[1] | ||
| bench = head | ||
| worker = tuple(nodelist[1:]) | ||
| else: | ||
| head = nodelist[0] | ||
| bench = head | ||
| infra = head | ||
| worker = tuple(nodelist[:]) | ||
|
|
||
| return cls(head=head, bench=bench, infra=infra, worker=worker) |
There was a problem hiding this comment.
Validate num_nodes >= 1 to avoid IndexError.
nodelist[0] will throw an IndexError when num_nodes is 0. A clear ValueError would make dry-run errors actionable.
✅ Suggested guard
@@
def from_mock(
cls,
num_nodes: int,
etcd_nats_dedicated_node: bool = False,
) -> "Nodes":
@@
- nodelist = [f"node-{i:02d}" for i in range(1, num_nodes + 1)]
+ if num_nodes < 1:
+ raise ValueError("num_nodes must be >= 1")
+ nodelist = [f"node-{i:02d}" for i in range(1, num_nodes + 1)]📝 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.
| @classmethod | |
| def from_mock( | |
| cls, | |
| num_nodes: int, | |
| etcd_nats_dedicated_node: bool = False, | |
| ) -> "Nodes": | |
| """Create mock Nodes for dry-run mode. | |
| Generates node names like node-01, node-02, etc. | |
| Args: | |
| num_nodes: Total number of nodes to simulate | |
| etcd_nats_dedicated_node: If True, dedicate first node for etcd/nats | |
| """ | |
| nodelist = [f"node-{i:02d}" for i in range(1, num_nodes + 1)] | |
| if etcd_nats_dedicated_node: | |
| if num_nodes < 2: | |
| raise ValueError("etcd_nats_dedicated_node requires at least 2 nodes") | |
| infra = nodelist[0] | |
| head = nodelist[1] | |
| bench = head | |
| worker = tuple(nodelist[1:]) | |
| else: | |
| head = nodelist[0] | |
| bench = head | |
| infra = head | |
| worker = tuple(nodelist[:]) | |
| return cls(head=head, bench=bench, infra=infra, worker=worker) | |
| `@classmethod` | |
| def from_mock( | |
| cls, | |
| num_nodes: int, | |
| etcd_nats_dedicated_node: bool = False, | |
| ) -> "Nodes": | |
| """Create mock Nodes for dry-run mode. | |
| Generates node names like node-01, node-02, etc. | |
| Args: | |
| num_nodes: Total number of nodes to simulate | |
| etcd_nats_dedicated_node: If True, dedicate first node for etcd/nats | |
| """ | |
| if num_nodes < 1: | |
| raise ValueError("num_nodes must be >= 1") | |
| nodelist = [f"node-{i:02d}" for i in range(1, num_nodes + 1)] | |
| if etcd_nats_dedicated_node: | |
| if num_nodes < 2: | |
| raise ValueError("etcd_nats_dedicated_node requires at least 2 nodes") | |
| infra = nodelist[0] | |
| head = nodelist[1] | |
| bench = head | |
| worker = tuple(nodelist[1:]) | |
| else: | |
| head = nodelist[0] | |
| bench = head | |
| infra = head | |
| worker = tuple(nodelist[:]) | |
| return cls(head=head, bench=bench, infra=infra, worker=worker) |
🤖 Prompt for AI Agents
In `@src/srtctl/core/runtime.py` around lines 80 - 109, The from_mock classmethod
currently constructs nodelist without checking num_nodes and will IndexError
when num_nodes==0; add an explicit validation at the start of from_mock (before
building nodelist) to raise ValueError("num_nodes must be >= 1") when num_nodes
< 1 so callers get a clear error; keep the existing etcd_nats_dedicated_node
check for num_nodes < 2 as-is and reference the same symbols: from_mock,
nodelist, etcd_nats_dedicated_node, head, infra, worker.
| # Create placeholder log directory (don't actually create it) | ||
| log_dir = Path("./dry-run-outputs") / job_id / "logs" | ||
|
|
There was a problem hiding this comment.
Keep dry‑run paths absolute to match RuntimeContext’s contract.
The class docstring says all paths are absolute; Path("./dry-run-outputs") is relative. Consider resolving it to an absolute path for consistency.
📌 Suggested adjustment
- log_dir = Path("./dry-run-outputs") / job_id / "logs"
+ log_dir = (Path.cwd() / "dry-run-outputs" / job_id / "logs").resolve()📝 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.
| # Create placeholder log directory (don't actually create it) | |
| log_dir = Path("./dry-run-outputs") / job_id / "logs" | |
| # Create placeholder log directory (don't actually create it) | |
| log_dir = (Path.cwd() / "dry-run-outputs" / job_id / "logs").resolve() | |
🤖 Prompt for AI Agents
In `@src/srtctl/core/runtime.py` around lines 342 - 344, The dry-run log_dir is
created using a relative Path("./dry-run-outputs") which violates
RuntimeContext’s contract that all paths be absolute; update the code that sets
log_dir (the variable in the runtime logic that uses job_id) to produce an
absolute path (e.g., resolve or join against an absolute base) so log_dir is
absolute before further use and continue to include the job_id/"logs" suffix;
ensure the change is made where log_dir is defined so any other uses of log_dir
or RuntimeContext observe an absolute path.
| # Build container mounts (simplified for dry-run) | ||
| container_mounts: dict[Path, Path] = { | ||
| log_dir: Path("/logs"), | ||
| } | ||
| if not is_hf_model: | ||
| container_mounts[model_path] = Path("/model") | ||
|
|
There was a problem hiding this comment.
Dry‑run omits cluster/user mounts from container_mounts.
Only /logs and /model are shown, so the enhanced dry‑run plan can be misleading when default_mounts, extra_mount, or config.container_mounts are set. Consider mirroring the mount assembly from from_config while skipping existence checks.
🤖 Prompt for AI Agents
In `@src/srtctl/core/runtime.py` around lines 354 - 360, The dry-run currently
builds container_mounts only with log_dir and model_path, which omits mounts
from default_mounts, extra_mount, and config.container_mounts; update the
dry-run assembly in runtime.py to mirror the logic used by from_config: start
with default_mounts, then apply config.container_mounts and extra_mount
(respecting keys/targets), and finally add the log_dir and model_path entries,
but skip any filesystem existence checks—ensure you update the container_mounts
dict construction and reuse the same mount key/Path normalization used by
from_config to keep behavior consistent.
| class ContainersField(fields.Field): | ||
| """Marshmallow field for containers that accepts both string and dict formats. | ||
|
|
||
| Handles both: | ||
| - Old format: {"sglang": "/path/to/sglang.sqsh"} | ||
| - New format: {"sglang": {"path": "/path/to/sglang.sqsh", "source": "docker://..."}} | ||
|
|
||
| Always deserializes to dict[str, ContainerEntry] internally. | ||
| """ | ||
|
|
||
| def _deserialize( | ||
| self, | ||
| value: Any, | ||
| attr: str | None, | ||
| data: Mapping[str, Any] | None, | ||
| **kwargs, | ||
| ) -> dict[str, ContainerEntry] | None: | ||
| if value is None: | ||
| return None | ||
| if not isinstance(value, dict): | ||
| raise ValidationError(f"Expected dict for containers, got {type(value).__name__}") | ||
|
|
||
| result: dict[str, ContainerEntry] = {} | ||
| for name, entry in value.items(): | ||
| if isinstance(entry, str): | ||
| # Old format: just a path string | ||
| result[name] = ContainerEntry(path=entry, source=None) | ||
| elif isinstance(entry, dict): | ||
| # New format: dict with path and optional source | ||
| if "path" not in entry: | ||
| raise ValidationError(f"Container '{name}' dict format requires 'path' field") | ||
| result[name] = ContainerEntry( | ||
| path=entry["path"], | ||
| source=entry.get("source"), | ||
| ) | ||
| else: | ||
| raise ValidationError( | ||
| f"Container '{name}' must be a string path or dict with 'path' field, got {type(entry).__name__}" | ||
| ) |
There was a problem hiding this comment.
Validate path/source types in ContainersField.
Right now non‑string values are accepted and stored, which can surface as confusing errors later. Consider explicit type checks with a ValidationError.
✅ Suggested validation
@@
elif isinstance(entry, dict):
# New format: dict with path and optional source
if "path" not in entry:
raise ValidationError(f"Container '{name}' dict format requires 'path' field")
+ if not isinstance(entry["path"], str):
+ raise ValidationError(f"Container '{name}' path must be a string")
+ if "source" in entry and entry["source"] is not None and not isinstance(entry["source"], str):
+ raise ValidationError(f"Container '{name}' source must be a string or null")
result[name] = ContainerEntry(
path=entry["path"],
source=entry.get("source"),
)🤖 Prompt for AI Agents
In `@src/srtctl/core/schema.py` around lines 195 - 233, In
ContainersField._deserialize, add explicit type validation for path and source
to avoid accepting non-string values: when handling the string entry case in
ContainersField, ensure the entry is a str before creating
ContainerEntry(path=...), and when handling the dict case (in
ContainersField._deserialize for entry dict), validate that entry["path"] exists
and is a str and that entry.get("source") is either None or a str; if these
checks fail, raise a ValidationError with a clear message referencing the
container name. Keep using ContainerEntry(path=..., source=...) once types are
confirmed and update the ValidationError messages to indicate the expected
types.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/srtctl/cli/mixins/frontend_stage.py (2)
69-71:⚠️ Potential issue | 🟠 MajorReplace the
backend_processesstub with a real abstract body.The ellipsis body implicitly returns
None, which matches the CI type-check failure. Use an explicitraise NotImplementedError(or mark as abstract) to satisfy the return type.🔧 Suggested fix
`@property` def backend_processes(self) -> list["Process"]: """Compute physical process topology from endpoints (cached).""" - ... + raise NotImplementedError
186-200:⚠️ Potential issue | 🟠 MajorCustom frontend + nginx topology mismatch on multi-node.
With
enable_multiple_frontendson a multi-node run, topology expects frontends on non-head nodes, but custom frontend always starts on the head. That leaves nginx with no upstreams. Consider forcing a single-frontend topology (no nginx) fortype="custom"or explicitly include the head in upstreams.🔧 Suggested fix (force single-frontend topology for custom)
- topology = self._compute_frontend_topology() + if self.config.frontend.type == "custom": + topology = FrontendTopology( + nginx_node=None, + frontend_nodes=[self.runtime.nodes.head], + frontend_port=8000, + public_port=8000, + ) + else: + topology = self._compute_frontend_topology()src/srtctl/core/schema.py (1)
986-1048:⚠️ Potential issue | 🟠 MajorAdd validation for custom benchmark command in
__post_init__.
BenchmarkType.CUSTOMrequires acommandfield, but there's no validation inSrtConfig.__post_init__(). This creates an inconsistency with frontend validation (which runs at initialization) and causes errors to surface later at runtime instead of failing fast. Custom benchmarks are validated only when the runner is instantiated, missing the opportunity to catch configuration errors early.Implement
_validate_benchmark()following the same pattern as_validate_frontend():Suggested fix
def __post_init__(self): """Validate configuration after initialization.""" self._validate_profiling() self._validate_frontend() + self._validate_benchmark() @@ def _validate_frontend(self): """Validate frontend configuration.""" if self.frontend.type == "custom" and not self.frontend.command: raise ValidationError("frontend.command is required when frontend.type is 'custom'") + + def _validate_benchmark(self): + """Validate benchmark configuration.""" + if self.benchmark.type == "custom" and not self.benchmark.command: + raise ValidationError("benchmark.command is required when benchmark.type is 'custom'")
🤖 Fix all issues with AI agents
In `@docs/architecture.md`:
- Around line 212-214: Update the documentation's class example for
SweepOrchestrator to match the real implementation by adding the missing
PostProcessStageMixin to the class inheritance list; locate the
SweepOrchestrator class declaration in the docs and include
PostProcessStageMixin alongside WorkerStageMixin, SidecarStageMixin,
FrontendStageMixin, and BenchmarkStageMixin so the doc example mirrors the
actual code.
In `@examples/nemo_dynamo_thompson_sampling.yaml`:
- Around line 19-47: The container commands (router's command in
sidecars.router, processor's command in sidecars.processor, and frontend's
command in frontend.command) assume /logs exists; modify each command to create
the directory before starting (e.g., run a mkdir -p /logs step or equivalent
init) and ensure proper permissions so the env vars ROUTER_METRICS_CSV,
PROCESSOR_METRICS_CSV and FRONTEND_TPS_CSV can be written; update the three
commands to perform the directory creation (and any chown/chmod if needed) prior
to launching the Python process.
- Around line 48-57: Remove the unsafe default by deleting or setting to false
the trust-remote-code entry under the sglang_config -> aggregated block for the
served-model-name "llama-3.3-70b"; specifically update the backend sglang_config
aggregated section to omit or change trust-remote-code so the example no longer
enables remote code execution for that model.
In `@src/srtctl/cli/do_sweep.py`:
- Around line 230-234: start_sidecars() returns a list of ManagedProcess but
registry.add_processes expects a dict; change the call site in do_sweep.py to
convert sidecar_procs into a dict keyed by process name before passing to
registry.add_processes (or alternatively update start_sidecars to return
dict[str, ManagedProcess]); reference the variables/functions sidecar_procs,
start_sidecars, and registry.add_processes and ensure the resulting mapping uses
each ManagedProcess.name as the key.
🧹 Nitpick comments (2)
examples/nemo_dynamo_thompson_sampling.yaml (1)
65-73: Pin the benchmark container image to a specific tag for reproducibility.Using an untagged container image reference (
nemo-nat) can drift and cause examples to fail. Sincenemo-natappears to be a custom or internally-managed image, ensure it is built and tagged with a specific version (e.g.,nemo-nat:v1.0) rather than relying on a mutablelatestor bare tag.docs/architecture.md (1)
1119-1120: Consider adding language specifiers to fenced code blocks.The ASCII art diagrams at lines 1119, 1176, and 1329 are flagged by markdownlint for missing language specifiers. While these are text diagrams rather than code, you can use
textorplaintextas the language identifier to satisfy the linter and improve rendering consistency.📝 Suggested fix
-``` +```text +------------------------------------------------------------------+ | ORCHESTRATION STAGES |Apply similar changes to the other two blocks at lines 1176 and 1329.
Also applies to: 1176-1177, 1329-1330
| @dataclass | ||
| class SweepOrchestrator(WorkerStageMixin, FrontendStageMixin, BenchmarkStageMixin): | ||
| class SweepOrchestrator(WorkerStageMixin, SidecarStageMixin, FrontendStageMixin, BenchmarkStageMixin): | ||
| config: SrtConfig |
There was a problem hiding this comment.
Documentation example is missing PostProcessStageMixin.
The actual code includes PostProcessStageMixin in the class definition, but this documentation example omits it. This inconsistency could confuse developers.
📝 Suggested fix
`@dataclass`
-class SweepOrchestrator(WorkerStageMixin, SidecarStageMixin, FrontendStageMixin, BenchmarkStageMixin):
+class SweepOrchestrator(WorkerStageMixin, SidecarStageMixin, FrontendStageMixin, BenchmarkStageMixin, PostProcessStageMixin):
config: SrtConfig📝 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.
| @dataclass | |
| class SweepOrchestrator(WorkerStageMixin, FrontendStageMixin, BenchmarkStageMixin): | |
| class SweepOrchestrator(WorkerStageMixin, SidecarStageMixin, FrontendStageMixin, BenchmarkStageMixin): | |
| config: SrtConfig | |
| `@dataclass` | |
| class SweepOrchestrator(WorkerStageMixin, SidecarStageMixin, FrontendStageMixin, BenchmarkStageMixin, PostProcessStageMixin): | |
| config: SrtConfig |
🤖 Prompt for AI Agents
In `@docs/architecture.md` around lines 212 - 214, Update the documentation's
class example for SweepOrchestrator to match the real implementation by adding
the missing PostProcessStageMixin to the class inheritance list; locate the
SweepOrchestrator class declaration in the docs and include
PostProcessStageMixin alongside WorkerStageMixin, SidecarStageMixin,
FrontendStageMixin, and BenchmarkStageMixin so the doc example mirrors the
actual code.
| # Custom Dynamo components via sidecars | ||
| sidecars: | ||
| router: | ||
| # Thompson Sampling router with KV overlap awareness | ||
| command: > | ||
| python3 /workspace/custom_dynamo/router.py | ||
| --block-size 64 | ||
| --router-type kv | ||
| --affinity-base 0.30 | ||
| --temp-base 1.0 | ||
| --lints-lambda 1.0 | ||
| env: | ||
| ROUTER_METRICS_CSV: "/logs/router_metrics.csv" | ||
|
|
||
| processor: | ||
| # Workload-aware processor | ||
| command: "python3 /workspace/custom_dynamo/processor.py --model /model --enable-router" | ||
| env: | ||
| PROCESSOR_METRICS_CSV: "/logs/processor_requests.csv" | ||
|
|
||
| frontend: | ||
| # Custom frontend with prefix hints support | ||
| type: custom | ||
| command: "python3 /workspace/custom_dynamo/frontend.py" | ||
| env: | ||
| FRONTEND_MODEL_MAPPING: '{"llama-3.3-70b": "/model"}' | ||
| FRONTEND_TPS_INTERVAL: "5" | ||
| FRONTEND_TPS_CSV: "/logs/frontend_throughput.csv" | ||
|
|
There was a problem hiding this comment.
Create /logs before writing metrics to avoid runtime failures.
If /logs doesn’t exist or isn’t writable in these containers, metrics writes can fail. Consider ensuring the directory exists in each command.
🛠️ Example fix (ensure /logs exists before running)
- command: >
- python3 /workspace/custom_dynamo/router.py
+ command: >
+ bash -lc 'mkdir -p /logs && python3 /workspace/custom_dynamo/router.py
--block-size 64
--router-type kv
--affinity-base 0.30
--temp-base 1.0
- --lints-lambda 1.0
+ --lints-lambda 1.0'- command: "python3 /workspace/custom_dynamo/processor.py --model /model --enable-router"
+ command: "bash -lc 'mkdir -p /logs && python3 /workspace/custom_dynamo/processor.py --model /model --enable-router'"- command: "python3 /workspace/custom_dynamo/frontend.py"
+ command: "bash -lc 'mkdir -p /logs && python3 /workspace/custom_dynamo/frontend.py'"📝 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.
| # Custom Dynamo components via sidecars | |
| sidecars: | |
| router: | |
| # Thompson Sampling router with KV overlap awareness | |
| command: > | |
| python3 /workspace/custom_dynamo/router.py | |
| --block-size 64 | |
| --router-type kv | |
| --affinity-base 0.30 | |
| --temp-base 1.0 | |
| --lints-lambda 1.0 | |
| env: | |
| ROUTER_METRICS_CSV: "/logs/router_metrics.csv" | |
| processor: | |
| # Workload-aware processor | |
| command: "python3 /workspace/custom_dynamo/processor.py --model /model --enable-router" | |
| env: | |
| PROCESSOR_METRICS_CSV: "/logs/processor_requests.csv" | |
| frontend: | |
| # Custom frontend with prefix hints support | |
| type: custom | |
| command: "python3 /workspace/custom_dynamo/frontend.py" | |
| env: | |
| FRONTEND_MODEL_MAPPING: '{"llama-3.3-70b": "/model"}' | |
| FRONTEND_TPS_INTERVAL: "5" | |
| FRONTEND_TPS_CSV: "/logs/frontend_throughput.csv" | |
| # Custom Dynamo components via sidecars | |
| sidecars: | |
| router: | |
| # Thompson Sampling router with KV overlap awareness | |
| command: > | |
| bash -lc 'mkdir -p /logs && python3 /workspace/custom_dynamo/router.py | |
| --block-size 64 | |
| --router-type kv | |
| --affinity-base 0.30 | |
| --temp-base 1.0 | |
| --lints-lambda 1.0' | |
| env: | |
| ROUTER_METRICS_CSV: "/logs/router_metrics.csv" | |
| processor: | |
| # Workload-aware processor | |
| command: "bash -lc 'mkdir -p /logs && python3 /workspace/custom_dynamo/processor.py --model /model --enable-router'" | |
| env: | |
| PROCESSOR_METRICS_CSV: "/logs/processor_requests.csv" | |
| frontend: | |
| # Custom frontend with prefix hints support | |
| type: custom | |
| command: "bash -lc 'mkdir -p /logs && python3 /workspace/custom_dynamo/frontend.py'" | |
| env: | |
| FRONTEND_MODEL_MAPPING: '{"llama-3.3-70b": "/model"}' | |
| FRONTEND_TPS_INTERVAL: "5" | |
| FRONTEND_TPS_CSV: "/logs/frontend_throughput.csv" |
🤖 Prompt for AI Agents
In `@examples/nemo_dynamo_thompson_sampling.yaml` around lines 19 - 47, The
container commands (router's command in sidecars.router, processor's command in
sidecars.processor, and frontend's command in frontend.command) assume /logs
exists; modify each command to create the directory before starting (e.g., run a
mkdir -p /logs step or equivalent init) and ensure proper permissions so the env
vars ROUTER_METRICS_CSV, PROCESSOR_METRICS_CSV and FRONTEND_TPS_CSV can be
written; update the three commands to perform the directory creation (and any
chown/chmod if needed) prior to launching the Python process.
| backend: | ||
| type: sglang | ||
| sglang_config: | ||
| aggregated: | ||
| tp: 4 | ||
| served-model-name: "llama-3.3-70b" | ||
| trust-remote-code: true | ||
| mem-fraction-static: 0.9 | ||
| enable-metrics: true | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
trust-remote-code sglang security best practices recommendations
💡 Result:
What trust_remote_code means (in SGLang context)
- SGLang’s Transformers fallback can run Hugging Face Hub models that require
trust_remote_code=True, i.e., it may download and execute model-provided Python code to load/run the model architecture. Treat this like running third‑party code in your inference environment. [1]
Security best practices / recommendations
1) Avoid trust_remote_code=True unless you must
- Prefer models that work without remote code (standard
transformersarchitectures) or use an implementation that doesn’t require executing Hub code. [2], [4] - If you’re selecting models for a managed/enterprise environment, note some platforms disallow
trust_remote_codemodels unless explicitly verified/trusted. [3]
2) If you must enable it, pin and verify exactly what you’re running
- Pin a specific revision/commit hash (not a floating branch like
main) so a repo update can’t silently change executed code. This is explicitly recommended in the Transformers security guidance. [2] - Inspect the repository’s Python modeling files (the code that will run) before enabling remote code. [2]
3) Prefer safe weight formats; block unsafe deserialization
- Use Safetensors and configure loading to refuse pickle-based weights (e.g.,
use_safetensorsin Transformers) to reduce arbitrary code execution risk from weight files. [2], [4]
4) Run SGLang/Transformers in a sandboxed runtime
Because remote code is still just code execution, isolate it:
- Run in a container/VM with a read-only filesystem where possible, non-root user, minimal Linux capabilities, and restrictive seccomp/AppArmor/SELinux profiles (principle of least privilege).
- Limit egress network access (or disable entirely) so malicious code can’t exfiltrate data or fetch payloads.
- Ensure no secrets (cloud creds, SSH keys, prod tokens) are available in the runtime environment.
5) Use trust signals and scanning—but don’t rely on them alone
- Prefer models from trusted/verified organizations, high adoption, and with transparent repo contents. [3], [4]
- Be aware that real-world incidents and research have shown malicious artifacts can appear in model repos, and scanners can have false positives/negatives—so combine scanning with pinning + isolation + review. [5], [6]
Sources
[1] SGLang Docs – Transformers fallback in SGLang (Remote code section) (docs.sglang.ai)
[2] Hugging Face Transformers – Security Policy (remote code + pin revision; safetensors recommendation) (github.com)
[3] Hugging Face on Azure – Security & Compliance (trust_remote_code not allowed unless verified/trusted) (huggingface.co)
[4] Hugging Face FAQ – Safetensors + trust_remote_code is false by default; modeling code visibility (github.com)
[5] Ars Technica – Example of malicious code hosted; discussion of pickle risks and mitigations like safetensors/scanning (arstechnica.com)
[6] JFrog – On malicious ML models and scanning limitations / obfuscation issues (jfrog.com)
Citations:
- 1: https://docs.sglang.ai/supported_models/transformers_fallback.html?utm_source=openai
- 2: https://github.com/huggingface/transformers/security/policy?utm_source=openai
- 3: https://huggingface.co/docs/microsoft-azure/security?utm_source=openai
- 4: https://github.com/huggingface/faq?utm_source=openai
- 5: https://arstechnica.com/security/2024/03/hugging-face-the-github-of-ai-hosted-code-that-backdoored-user-devices/?utm_source=openai
- 6: https://jfrog.com/blog/jfrog-and-hugging-face-join-forces/?utm_source=openai
🌐 Web query:
trust_remote_code Hugging Face security risks arbitrary code execution
💡 Result:
trust_remote_code=True (e.g., in transformers.AutoModel.from_pretrained(...)) tells Transformers to download and execute Python code from the model repository (custom modeling_*.py, config, etc.). That means a malicious model repo—or a legitimate repo that later gets compromised—can run arbitrary code on your machine with your user’s privileges. Hugging Face/Transformers explicitly recommends reviewing the remote modeling files before using this flag and pinning a specific revision/commit to avoid silently pulling new code later. [1]
Why this is risky (common real-world paths to arbitrary code execution)
- Malicious repositories: security researchers have found Hugging Face Hub repos that act as backdoors / run code when users load them. [2][3]
- Unsafe serialization / deserialization: even without
trust_remote_code, loading ML artifacts can lead to arbitrary code execution when unsafe formats are used (notablypickle). NVD has published multiple Transformers-related RCE issues involving deserialization/validation problems. [4]
Practical mitigations
- Do not enable
trust_remote_codeunless you fully trust the repo/author and have reviewed the code. [1] - Pin the model to an exact commit hash (
revision=...) when you must trust remote code. [1] - Prefer
safetensorsand enforce it (e.g.,use_safetensors=True) to reduce pickle-style code-execution risk. [1] - Load/run untrusted models in sandboxed environments (container/VM, low privileges, no secrets on disk, restricted network/egress).
Sources: Transformers security policy guidance on trust_remote_code, revisions, and safetensors [1]; reporting on malicious Hugging Face models/backdoors [2][3]; NVD example of Transformers RCE via deserialization/insufficient validation [4].
[1] (github.com)
[2] (bleepingcomputer.com)
[3] (arstechnica.com)
[4] (nvd.nist.gov)
Citations:
- 1: https://github.com/huggingface/transformers/security/policy?utm_source=openai
- 2: https://www.bleepingcomputer.com/news/security/malicious-ai-models-on-hugging-face-backdoor-users-machines/?utm_source=openai
- 3: https://arstechnica.com/security/2024/03/hugging-face-the-github-of-ai-hosted-code-that-backdoored-user-devices/?utm_source=openai
- 4: https://nvd.nist.gov/vuln/detail/CVE-2024-11394?utm_source=openai
🏁 Script executed:
cat -n examples/nemo_dynamo_thompson_sampling.yaml | head -60Repository: ishandhanani/srt-slurm
Length of output: 2123
Remove trust-remote-code: true — this model does not require it and poses an unnecessary security risk.
Llama-3.3-70B-Instruct is a standard Hugging Face model that does not require remote code execution. Setting trust-remote-code: true in an example file propagates an unsafe default; users copying this configuration will unnecessarily expose their environments to arbitrary code execution from model repositories. Hugging Face security guidance recommends defaulting to false and only enabling it when strictly required for non-standard models, with pinned revisions and thorough code review.
Remove unnecessary setting
- trust-remote-code: true
+ trust-remote-code: false🤖 Prompt for AI Agents
In `@examples/nemo_dynamo_thompson_sampling.yaml` around lines 48 - 57, Remove the
unsafe default by deleting or setting to false the trust-remote-code entry under
the sglang_config -> aggregated block for the served-model-name "llama-3.3-70b";
specifically update the backend sglang_config aggregated section to omit or
change trust-remote-code so the example no longer enables remote code execution
for that model.
| # Stage 3: Sidecars (optional - runs after workers, before frontend) | ||
| if self.config.sidecars: | ||
| logger.info("Starting sidecars") | ||
| sidecar_procs = self.start_sidecars() | ||
| registry.add_processes(sidecar_procs) |
There was a problem hiding this comment.
Type mismatch: add_processes expects a dict, not a list.
The pipeline failure confirms this: start_sidecars() returns list[ManagedProcess], but registry.add_processes() expects dict[str, ManagedProcess]. Line 228 works because start_all_workers() returns a dict.
🐛 Proposed fix: convert list to dict keyed by process name
# Stage 3: Sidecars (optional - runs after workers, before frontend)
if self.config.sidecars:
logger.info("Starting sidecars")
sidecar_procs = self.start_sidecars()
- registry.add_processes(sidecar_procs)
+ for proc in sidecar_procs:
+ registry.add_process(proc)Alternatively, if you prefer the batch method, convert to dict:
registry.add_processes({proc.name: proc for proc in sidecar_procs})🧰 Tools
🪛 GitHub Actions: CI
[error] 234-234: invalid-argument-type: Argument to bound method add_processes is incorrect. Expected dict[str, ManagedProcess], found list[ManagedProcess].
🤖 Prompt for AI Agents
In `@src/srtctl/cli/do_sweep.py` around lines 230 - 234, start_sidecars() returns
a list of ManagedProcess but registry.add_processes expects a dict; change the
call site in do_sweep.py to convert sidecar_procs into a dict keyed by process
name before passing to registry.add_processes (or alternatively update
start_sidecars to return dict[str, ManagedProcess]); reference the
variables/functions sidecar_procs, start_sidecars, and registry.add_processes
and ensure the resulting mapping uses each ManagedProcess.name as the key.
|
@coderabbitai review again - i made fixes |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/srtctl/cli/mixins/frontend_stage.py (1)
179-199:⚠️ Potential issue | 🟠 MajorCustom frontend can be misrouted when nginx topology is enabled.
Topology is computed (and nginx started) before the custom branch. If multiple frontends are enabled, nginx will route totopology.frontend_nodes, but the custom frontend starts only on the head node. That leads to nginx proxying to nodes with no frontend.🛠️ Suggested fix
- topology = self._compute_frontend_topology() + if self.config.frontend.type == "custom": + topology = FrontendTopology( + nginx_node=None, + frontend_nodes=[self.runtime.nodes.head], + frontend_port=8000, + public_port=8000, + ) + else: + topology = self._compute_frontend_topology()
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 42-46: The CI job runs ty via uvx without installing project deps,
so ty cannot resolve imports; update the job to install project dependencies
before running type checks by adding a step that runs the same dependency sync
used in lint/test (run the command uv sync --dev) prior to the existing Run ty
step, then run uvx ty check src/ in that environment (keep the existing "Set up
Python" step intact); this ensures imports like jinja2 are available to the ty
run.
In `@Makefile`:
- Around line 13-16: The Makefile currently uses the isolated runner "uvx" for
the type check step which prevents project dependencies (like jinja2) from being
resolved; update the target that runs "uvx ty check src/" to use the project
environment runner "uv" instead (i.e., change the command from "uvx ty check
src/" to "uv ty check src/") so the "ty" invocation in the Makefile can import
modules used by src/srtctl/cli/submit.py and
src/srtctl/cli/mixins/frontend_stage.py.
In `@src/srtctl/cli/mixins/frontend_stage.py`:
- Around line 233-235: fe_config.container may be a ContainerEntry/alias and
must be resolved to a real image path before being passed to start_srun_process;
replace the direct use of fe_config.container with the same container-alias
resolution used in the benchmark-stage (i.e., resolve the ContainerEntry/alias
to a concrete image string and fall back to str(self.runtime.container_image) if
resolution fails) and use that resolved value for container_image wherever
start_srun_process is invoked (also apply the same resolution in the second
block that mirrors lines 256-262).
In `@src/srtctl/templates/job_script_minimal.j2`:
- Around line 78-89: The uv installer invocation is unpinned in the job script
(see the curl | sh line) which makes job submissions non-reproducible; update
the installer call in src/srtctl/templates/job_script_minimal.j2 to pin a stable
uv release (e.g., pass the --version flag to the installer like -- --version
0.1.x) or switch to using a pre-bundled uv binary on the cluster, and ensure
subsequent uses (the uv pip install line that writes to "${SRTCTL_INSTALL_DIR}"
using "${SRTCTL_SOURCE}") rely on that pinned installer so all runs install the
same uv version.
🧹 Nitpick comments (3)
configs/nat-setup.sh (1)
34-45: Sed patches are not idempotent and may silently fail.The
sed -icommands will run every time the script executes. If the patterns have already been replaced (or don't exist),sedwill silently succeed but the file won't change. This isn't necessarily a bug, but:
- If run twice, the file gets processed twice (minor I/O overhead)
- If the upstream repo changes these patterns, the patches will silently fail
Consider adding guards or using a marker file to track that patches were applied:
💡 Suggested improvement for idempotent patching
# Fix Dynamo 0.8.0 API compatibility -echo "Patching Dynamo API compatibility..." +PATCH_MARKER="$NAT_DIR/.srtctl_patched" +if [ ! -f "$PATCH_MARKER" ]; then + echo "Patching Dynamo API compatibility..." # Remove static= parameter (was removed in 0.8.0) sed -i 's/@dynamo_worker(static=False)/@dynamo_worker()/g' "$NAT_DIR/external/dynamo/generalized/router.py" # ... rest of sed commands + touch "$PATCH_MARKER" +else + echo "Dynamo API compatibility patches already applied" +fitests/test_container_pull.py (1)
411-413: Missing trailing newline at end of file.The file should end with a newline character per standard Python conventions (and most linters/formatters will add it).
tests/test_sidecars.py (1)
509-511: Missing trailing newline at end of file.Similar to the container pull test file, this file should end with a newline character.
| - name: Set up Python | ||
| run: uv python install 3.10 | ||
|
|
||
| - name: Install dependencies | ||
| run: uv sync --dev | ||
|
|
||
| - name: Run ty | ||
| run: uv run ty check src/srtctl/ | ||
| continue-on-error: true # ty is still experimental | ||
| run: uvx ty check src/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/ci.yaml | head -60Repository: ishandhanani/srt-slurm
Length of output: 1621
🏁 Script executed:
# Check if jinja2 is in the project dependencies
fd pyproject.toml requirements.txt -x cat {}Repository: ishandhanani/srt-slurm
Length of output: 169
🏁 Script executed:
# Look for jinja2 imports in the codebase
rg "import jinja2|from jinja2" src/Repository: ishandhanani/srt-slurm
Length of output: 240
🏁 Script executed:
# Check the project dependencies to understand what needs to be available
cat pyproject.toml | head -50Repository: ishandhanani/srt-slurm
Length of output: 1510
🏁 Script executed:
# Check if there's a uv.lock file to understand the project setup
head -20 uv.lock 2>/dev/null || echo "No uv.lock found"Repository: ishandhanani/srt-slurm
Length of output: 82
🏁 Script executed:
# Look at the learnings more carefully - check if py.typed or other type hints setup
rg "jinja2" pyproject.tomlRepository: ishandhanani/srt-slurm
Length of output: 86
🏁 Script executed:
# Check if there are any recent CI logs or test results mentioned
git log --oneline -10 2>/dev/null | head -20Repository: ishandhanani/srt-slurm
Length of output: 76
🏁 Script executed:
# Verify the actual behavior difference: check what uvx does vs uv run
# Look at any documentation or comments in the workflow about this choice
rg "uvx|uv run" .github/workflows/Repository: ishandhanani/srt-slurm
Length of output: 514
Typecheck job runs without project deps (breaks ty).
With no uv sync --dev, ty runs in isolation via uvx and can't resolve imports like jinja2, causing CI failures. Install dependencies and run ty in the project environment like the lint and test jobs do.
🛠️ Suggested fix
- name: Set up Python
run: uv python install 3.10
+ - name: Install dependencies
+ run: uv sync --dev
+
- name: Run ty
- run: uvx ty check src/
+ run: uv run ty check src/🤖 Prompt for AI Agents
In @.github/workflows/ci.yaml around lines 42 - 46, The CI job runs ty via uvx
without installing project deps, so ty cannot resolve imports; update the job to
install project dependencies before running type checks by adding a step that
runs the same dependency sync used in lint/test (run the command uv sync --dev)
prior to the existing Run ty step, then run uvx ty check src/ in that
environment (keep the existing "Set up Python" step intact); this ensures
imports like jinja2 are available to the ty run.
| uv run ruff check src/srtctl/ | ||
| uv run ruff format src/srtctl/ | ||
| uv run ty check src/srtctl/ || true | ||
| uvx ty check src/ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the repository structure
git ls-files | head -20Repository: ishandhanani/srt-slurm
Length of output: 561
🏁 Script executed:
# Find and examine the Makefile
find . -name "Makefile" -type f -exec cat -n {} +Repository: ishandhanani/srt-slurm
Length of output: 6997
🏁 Script executed:
# Look for project configuration files
find . -maxdepth 2 -name "pyproject.toml" -o -name "uv.lock" -o -name "setup.py" | head -20Repository: ishandhanani/srt-slurm
Length of output: 82
🏁 Script executed:
# Check for any CI/CD configuration
find . -maxdepth 3 \( -name ".github" -o -name ".gitlab-ci.yml" -o -name "*.yml" -o -name "*.yaml" \) -type d | head -10Repository: ishandhanani/srt-slurm
Length of output: 75
🏁 Script executed:
# Read pyproject.toml to see dependencies
cat -n pyproject.tomlRepository: ishandhanani/srt-slurm
Length of output: 3449
🏁 Script executed:
# Check CI configuration
cat -n .github/workflows/ci.yamlRepository: ishandhanani/srt-slurm
Length of output: 3584
🏁 Script executed:
# Check if src/srtctl actually imports jinja2
rg "import jinja2|from jinja2" src/srtctl/Repository: ishandhanani/srt-slurm
Length of output: 240
🏁 Script executed:
# Also check for any jinja2 usage indirectly
rg "jinja2" src/srtctl/Repository: ishandhanani/srt-slurm
Length of output: 240
Use uv run ty check src/ instead of uvx ty check src/.
uvx runs in an isolated environment without project dependencies, so ty cannot resolve imports like jinja2 (which src/srtctl/cli/submit.py and src/srtctl/cli/mixins/frontend_stage.py depend on). Run ty in the project environment instead to ensure all dependencies are available.
Suggested fix
lint:
uv run ruff check src/srtctl/
uv run ruff format src/srtctl/
- uvx ty check src/
+ uv run ty check src/📝 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.
| uv run ruff check src/srtctl/ | |
| uv run ruff format src/srtctl/ | |
| uv run ty check src/srtctl/ || true | |
| uvx ty check src/ | |
| lint: | |
| uv run ruff check src/srtctl/ | |
| uv run ruff format src/srtctl/ | |
| uv run ty check src/ |
🤖 Prompt for AI Agents
In `@Makefile` around lines 13 - 16, The Makefile currently uses the isolated
runner "uvx" for the type check step which prevents project dependencies (like
jinja2) from being resolved; update the target that runs "uvx ty check src/" to
use the project environment runner "uv" instead (i.e., change the command from
"uvx ty check src/" to "uv ty check src/") so the "ty" invocation in the
Makefile can import modules used by src/srtctl/cli/submit.py and
src/srtctl/cli/mixins/frontend_stage.py.
| # Determine container image (frontend-specific > model.container) | ||
| container_image = fe_config.container or str(self.runtime.container_image) | ||
|
|
There was a problem hiding this comment.
Resolve custom frontend container aliases to a real path.
fe_config.container is likely a ContainerEntry/alias; passing it directly to start_srun_process can break container resolution. Mirror the benchmark-stage resolution before invoking srun.
🛠️ Suggested fix
- container_image = fe_config.container or str(self.runtime.container_image)
+ if fe_config.container is not None:
+ container_image = fe_config.container.get_path(self.runtime, ensure_exists=False)
+ else:
+ container_image = self.runtime.container_imageAlso applies to: 256-262
🤖 Prompt for AI Agents
In `@src/srtctl/cli/mixins/frontend_stage.py` around lines 233 - 235,
fe_config.container may be a ContainerEntry/alias and must be resolved to a real
image path before being passed to start_srun_process; replace the direct use of
fe_config.container with the same container-alias resolution used in the
benchmark-stage (i.e., resolve the ContainerEntry/alias to a concrete image
string and fall back to str(self.runtime.container_image) if resolution fails)
and use that resolved value for container_image wherever start_srun_process is
invoked (also apply the same resolution in the second block that mirrors lines
256-262).
| echo "Installing srtctl dependencies..." | ||
|
|
||
| # Install uv if not present (single binary, no dependencies) | ||
| if ! command -v uv &> /dev/null; then | ||
| echo "Installing uv package manager..." | ||
| curl -LsSf https://astral.sh/uv/install.sh | sh | ||
| export PATH="$HOME/.local/bin:$PATH" | ||
| fi | ||
|
|
||
| # Use uv to install srtctl (handles native extensions properly) | ||
| echo "Using uv to install srtctl..." | ||
| uv pip install --quiet --target="${SRTCTL_INSTALL_DIR}" "${SRTCTL_SOURCE}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the job script file to understand full context
cat -n src/srtctl/templates/job_script_minimal.j2Repository: ishandhanani/srt-slurm
Length of output: 4526
🏁 Script executed:
# Search for Dockerfile or container build files that might address this
fd -type f -name "Dockerfile*" -o -name "*.dockerfile" -o -name ".dockerignore"Repository: ishandhanani/srt-slurm
Length of output: 236
🏁 Script executed:
# Look for version pinning or uv installation elsewhere in the codebase
rg -i "uv.*install\|uv.*version\|astral.sh" --max-count=20Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Search for Docker files with proper fd syntax
fd DockerfileRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Search for .container files or related deployment configs
fd -e dockerfile -e dockerfile.txt -e containerfileRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look for setup.py, requirements.txt, pyproject.toml to understand dependencies
fd -e "requirements.txt" -e "setup.py" -e "pyproject.toml" -e "setup.cfg"Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Search for any CI/CD configuration
fd -path "*/.github/workflows/*.yml" -o -path "*/.github/workflows/*.yaml" -o -path "*/.gitlab-ci.yml" -o -path "*/Jenkinsfile"Repository: ishandhanani/srt-slurm
Length of output: 234
🏁 Script executed:
# Check README or docs for deployment information
fd -e "README.md" -e "INSTALL.md" -e "DEPLOYMENT.md"Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Search for any README files
find . -maxdepth 2 -iname "README*" -type fRepository: ishandhanani/srt-slurm
Length of output: 112
🏁 Script executed:
# Search for any container or deployment related files
find . -maxdepth 2 \( -iname "*docker*" -o -iname "*.container" -o -iname "*manifest*" \) -type fRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check for CI/CD files in common locations
ls -la .github/workflows/ 2>/dev/null || echo "No .github/workflows found"Repository: ishandhanani/srt-slurm
Length of output: 233
🏁 Script executed:
# Check root directory for common config files
ls -la | grep -E "requirements|setup|pyproject|docker"Repository: ishandhanani/srt-slurm
Length of output: 130
🏁 Script executed:
# Search more broadly for uv usage patterns in the codebase
rg "uv.*install|curl.*install" -i --type sh --type bashRepository: ishandhanani/srt-slurm
Length of output: 94
🏁 Script executed:
# Check if there's any version info or pinning for uv anywhere
rg "uv==|uv>=|version.*uv" -iRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Read the main README to understand deployment context
cat README.mdRepository: ishandhanani/srt-slurm
Length of output: 1271
🏁 Script executed:
# Read the CI configuration to understand how this is deployed
cat .github/workflows/ci.yamlRepository: ishandhanani/srt-slurm
Length of output: 2772
🏁 Script executed:
# Search for uv in shell/bash files more carefully
rg "uv\s+pip|curl.*uv|astral" -A 2 -B 2Repository: ishandhanani/srt-slurm
Length of output: 1614
🏁 Script executed:
# Check pyproject.toml for any uv or python version specifications
cat pyproject.tomlRepository: ishandhanani/srt-slurm
Length of output: 2770
Pin uv installer version to ensure reproducible job submissions.
This script installs uv via curl -LsSf https://astral.sh/uv/install.sh | sh without version pinning, which means different job submissions over time could install different uv versions. For an HPC benchmarking tool, this reduces reproducibility of sweep results. Pin the version (e.g., curl -LsSf https://astral.sh/uv/install.sh | sh -- --version 0.1.x) or pre-bundle uv on cluster nodes.
🤖 Prompt for AI Agents
In `@src/srtctl/templates/job_script_minimal.j2` around lines 78 - 89, The uv
installer invocation is unpinned in the job script (see the curl | sh line)
which makes job submissions non-reproducible; update the installer call in
src/srtctl/templates/job_script_minimal.j2 to pin a stable uv release (e.g.,
pass the --version flag to the installer like -- --version 0.1.x) or switch to
using a pre-bundled uv binary on the cluster, and ensure subsequent uses (the uv
pip install line that writes to "${SRTCTL_INSTALL_DIR}" using
"${SRTCTL_SOURCE}") rely on that pinned installer so all runs install the same
uv version.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/srtctl/core/health.py`:
- Around line 413-418: The project uses requests.get in
src/srtctl/core/health.py (see the health_url/requests.get usage and logger.info
paths) but the requests package has no inline types; add the types-requests stub
to the dev dependency group so the CI type-checker (uvx ty) can type-check src/.
Update pyproject.toml by adding "types-requests" to the [dependency-groups] dev
section (alongside other dev deps) and run the type check locally to confirm the
error is resolved.
🧹 Nitpick comments (4)
recipes/nat/thompson.yaml (1)
69-81: Pin the NeMo-Agent-Toolkit revision for reproducible benchmarks.
git clone --depth 1without a pinned tag/commit makes results nondeterministic and harder to reproduce. Consider pinning to a tag/commit and honoringNAT_REPO/NAT_BRANCHas documented.📌 Example pinning (replace with an actual tag/commit)
- git clone --depth 1 https://github.com/NVIDIA/NeMo-Agent-Toolkit.git /workspace/NeMo-Agent-Toolkit && + git clone --depth 1 --branch "${NAT_BRANCH:-vX.Y.Z}" "${NAT_REPO:-https://github.com/NVIDIA/NeMo-Agent-Toolkit.git}" /workspace/NeMo-Agent-Toolkit &&tests/test_benchmarks.py (2)
257-289: Test name is misleading—this tests multiple arguments, not multiline commands.The test is named
test_build_command_multilinebut the command template is a single-line command with multiple arguments:"uvx aiperf profile --url {nginx_url} --concurrency 128". Consider renaming totest_build_command_with_multiple_argsor similar for clarity.✏️ Suggested rename
- def test_build_command_multiline(self): - """Multiline commands are handled correctly.""" + def test_build_command_with_multiple_args(self): + """Commands with multiple arguments are handled correctly."""
315-471: Consider adding edge case tests for complete coverage.The shell operator tests are good but don't cover all operators in the implementation's
shell_operatorslist (line 97 of custom.py):
- Backtick (
`) for legacy command substitution- Redirection operators (
>,<)- The
shlex.splitValueError fallback (e.g., unclosed quotes like"echo 'unclosed)These are lower-priority edge cases but would improve test completeness.
src/srtctl/core/health.py (1)
363-386: Refactor to usematchstatement for frontend_type branching.The if/elif/else at lines 370–390 is a good candidate for a
matchstatement, which aligns with the project's Python 3.10+ style requirements and improves readability for pattern-based branching.♻️ Suggested refactor
- if frontend_type == "sglang": - health_url = f"http://{host}:{port}/workers" - logger.info( - "Polling %s every %.1fs for %d prefills and %d decodes (sglang frontend)", - health_url, - poll_interval, - n_prefill, - n_decode, - ) - elif frontend_type == "custom": - health_url = f"http://{host}:{port}/health" - logger.info( - "Polling %s every %.1fs (custom frontend, no worker count validation)", - health_url, - poll_interval, - ) - else: - health_url = f"http://{host}:{port}/health" - logger.info( - "Polling %s every %.1fs for %d prefills and %d decodes", - health_url, - poll_interval, - n_prefill, - n_decode, - ) + match frontend_type: + case "sglang": + health_url = f"http://{host}:{port}/workers" + logger.info( + "Polling %s every %.1fs for %d prefills and %d decodes (sglang frontend)", + health_url, + poll_interval, + n_prefill, + n_decode, + ) + case "custom": + health_url = f"http://{host}:{port}/health" + logger.info( + "Polling %s every %.1fs (custom frontend, no worker count validation)", + health_url, + poll_interval, + ) + case _: + health_url = f"http://{host}:{port}/health" + logger.info( + "Polling %s every %.1fs for %d prefills and %d decodes", + health_url, + poll_interval, + n_prefill, + n_decode, + )
| response = requests.get(health_url, timeout=5.0) | ||
| if response.status_code == 200: | ||
| # Custom frontend: just check for HTTP 200, no worker count validation | ||
| if frontend_type == "custom": | ||
| logger.info("Custom frontend is healthy (HTTP 200)") | ||
| return True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check project structure and find dependency files
echo "=== Finding dependency files ==="
fd -a -t f 'pyproject.toml|poetry.lock|uv.lock' 2>/dev/null || true
find . -maxdepth 2 -name "requirements*.txt" 2>/dev/null || true
echo ""
echo "=== Checking pyproject.toml for requests and types-requests ==="
if [ -f pyproject.toml ]; then
cat -n pyproject.toml | grep -A 50 -i "dependencies\|requests" || true
fi
echo ""
echo "=== Checking requirements files ==="
for f in requirements*.txt; do
if [ -f "$f" ]; then
echo "File: $f"
grep -i "requests" "$f" || echo " (no requests found)"
fi
done
echo ""
echo "=== Inspecting src/srtctl/core/health.py around lines 413-418 ==="
if [ -f "src/srtctl/core/health.py" ]; then
echo "File exists. Reading lines 410-420:"
sed -n '410,420p' "src/srtctl/core/health.py" | cat -n
fi
echo ""
echo "=== Checking imports in health.py ==="
head -50 "src/srtctl/core/health.py" | grep -i "import.*requests" || echo "(no requests import in first 50 lines)"Repository: ishandhanani/srt-slurm
Length of output: 3554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full ty configuration section ==="
if [ -f pyproject.toml ]; then
sed -n '82,120p' pyproject.toml | cat -n
fi
echo ""
echo "=== Check if types-requests is mentioned anywhere ==="
grep -r "types-requests" . 2>/dev/null || echo "(types-requests not found)"
echo ""
echo "=== Check entire pyproject.toml for stub packages or type checking config ==="
rg -n "stub|Stub|TYPE_CHECKING" pyproject.toml || trueRepository: ishandhanani/srt-slurm
Length of output: 749
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
echo "=== Check requests library metadata and type information ==="
python3 << 'EOF'
import sys
try:
import requests
print(f"requests version: {requests.__version__}")
print(f"requests location: {requests.__file__}")
# Check if requests has py.typed
import os
requests_dir = os.path.dirname(requests.__file__)
py_typed = os.path.join(requests_dir, "py.typed")
print(f"py.typed exists: {os.path.exists(py_typed)}")
# Check for __init__.pyi
init_pyi = os.path.join(requests_dir, "__init__.pyi")
print(f"__init__.pyi exists: {os.path.exists(init_pyi)}")
except Exception as e:
print(f"Error: {e}")
EOF
echo ""
echo "=== Look for type: ignore comments in health.py around requests usage ==="
sed -n '410,425p' "src/srtctl/core/health.py" | cat -nRepository: ishandhanani/srt-slurm
Length of output: 1261
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Check if there are any type: ignore comments for requests in health.py ==="
grep -n "type.*ignore.*requests\|requests.*type.*ignore" "src/srtctl/core/health.py" || echo "(none found)"
echo ""
echo "=== Check all imports in health.py ==="
head -30 "src/srtctl/core/health.py" | grep -n "^import\|^from"
echo ""
echo "=== Search for any requests usage across the codebase ==="
rg -c "requests\." "src/" | head -10Repository: ishandhanani/srt-slurm
Length of output: 583
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Check ty documentation and stub package handling ==="
# Since we need to verify if types-requests is the right fix
# Let's check if there are any other type-checking configuration issues
echo "=== Full pyproject.toml content to see all config ==="
wc -l pyproject.toml
echo ""
echo "Looking for any type-checking or typing configuration..."
rg -n "typing|TYPE_CHECKING|type.*check" pyproject.toml
echo ""
echo "=== Verify that the exact lines 413-418 match the snippet in review ==="
sed -n '413,418p' "src/srtctl/core/health.py" | cat -nRepository: ishandhanani/srt-slurm
Length of output: 833
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Check if requests library provides inline types ==="
python3 << 'EOF'
import inspect
try:
import requests
# Check if requests.get has type hints
sig = inspect.signature(requests.get)
print("requests.get signature:")
print(f" {sig}")
print(f" Annotations: {requests.get.__annotations__ if hasattr(requests.get, '__annotations__') else 'None'}")
except Exception as e:
print(f"Error: {e}")
EOF
echo ""
echo "=== Look for any existing workarounds (type: ignore comments) in other files using requests ==="
rg -B 2 "requests\." "src/" | grep -A 2 "type.*ignore\|requests" | head -30Repository: ishandhanani/srt-slurm
Length of output: 1677
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Check if types-requests should be added per best practices ==="
python3 << 'EOF'
# Verify that types-requests is the correct stub package for requests
import json
# Known fact: requests library uses typeshed stubs via types-requests package
print("Standard approach for typing requests:")
print("- requests library: provides no inline types (has empty __annotations__)")
print("- Solution: Add 'types-requests' to dev dependencies for type checkers")
print("- This is the standard stub package in typeshed")
EOF
echo ""
echo "=== Verify if there are CI/type-checking configurations ==="
find . -maxdepth 2 \( -name ".github" -o -name ".gitlab-ci.yml" -o -name "tox.ini" -o -name "Makefile" \) 2>/dev/null | head -5
ls -la | grep -E "\.github|\.gitlab|tox|Makefile" || echo "(no CI files found at root)"Repository: ishandhanani/srt-slurm
Length of output: 562
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Check GitHub workflows for type-checking ==="
find .github -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -5
echo ""
echo "=== Check if any CI workflow runs type checking ==="
for f in $(find .github -name "*.yml" -o -name "*.yaml" 2>/dev/null); do
echo "File: $f"
grep -n "ty\|type.*check\|mypy" "$f" || echo " (no type-checking found)"
done
echo ""
echo "=== Check Makefile for type-checking targets ==="
grep -n "ty\|type" Makefile || echo "(no type-checking targets found)"Repository: ishandhanani/srt-slurm
Length of output: 561
Add types-requests to dev dependencies for type-checking.
The requests library (v2.31.0+) lacks inline type hints and requires the types-requests stub package for type checkers like ty. The code at lines 413–418 uses requests.get() without type annotations, which blocks CI type-checking (see .github/workflows/ci.yaml:46 which runs uvx ty check src/). Add "types-requests" to the [dependency-groups] dev section in pyproject.toml.
🤖 Prompt for AI Agents
In `@src/srtctl/core/health.py` around lines 413 - 418, The project uses
requests.get in src/srtctl/core/health.py (see the health_url/requests.get usage
and logger.info paths) but the requests package has no inline types; add the
types-requests stub to the dev dependency group so the CI type-checker (uvx ty)
can type-check src/. Update pyproject.toml by adding "types-requests" to the
[dependency-groups] dev section (alongside other dev deps) and run the type
check locally to confirm the error is resolved.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@configs/nat-eval/eval_config.yml`:
- Line 15: The path specified by the tools_json_path setting in eval_config.yml
points to a non-existent file
('./examples/dynamo_integration/data/raw/banking/tools.json'); either add the
missing tools.json at that location or update the tools_json_path value to the
correct relative path for the existing tools file so evaluations can load it;
locate the tools_json_path key in eval_config.yml and modify it or add the
referenced tools.json resource accordingly.
- Around line 113-117: The dataset file path in the eval config is invalid:
update the dataset.file_path (or replace the dataset block) to point to an
existing file or add the missing JSON into the repo; specifically edit the
dataset entry (dataset._type, dataset.file_path, dataset.structure) in
eval_config.yml to either reference an existing example file under ./examples
(or correct subdirectory) or add the missing
./examples/dynamo_integration/data/agent_leaderboard_v2_test_subset.json file so
the path resolves.
In `@src/srtctl/cli/mixins/frontend_stage.py`:
- Around line 294-296: The long f-string that builds the shell command using
script_path exceeds 120 characters; split it into multiple shorter string parts
(using parentheses for implicit concatenation or '+' concatenation) so each line
stays under 120 chars, e.g. separate the "echo 'Running setup script:
{script_path}' && " segment from the "if [ -f '{script_path}' ]; then bash
'{script_path}'; else echo 'WARNING: {script_path} not found'; fi" segment and
reassemble them into the same variable (preserving the f-string interpolation
for script_path).
- Around line 237-239: Replace the runtime assertion with an explicit exception:
instead of using "assert fe_config.command is not None", raise a clear exception
(e.g., ValueError or RuntimeError) when fe_config.command is missing so the
check cannot be bypassed with -O; update the check in the frontend stage where
fe_config and its command field are validated (the spot that currently reads the
assert) to raise the exception with the message "Custom frontend requires
'command' field".
In `@src/srtctl/cli/mixins/sidecar_stage.py`:
- Around line 45-61: The return map from start_sidecars uses proc.name (which
becomes "sidecar_{name}") as keys but the docstring and callers expect the
original sidecar config keys; change the dictionary insertion in start_sidecars
to use the loop variable name as the key (processes[name] = proc) so keys match
config names (or, if you prefer the prefixed proc.name, update the docstring and
all callers to expect that new key format); key symbols: start_sidecars,
_start_sidecar, processes, proc.name.
- Around line 93-95: After splitting the sidecar command with shlex.split (the
line setting cmd = shlex.split(config.command)), guard against empty or
whitespace-only commands by checking if cmd is an empty list and raise a clear,
early error (e.g., ValueError or a CLI-specific exception) before proceeding to
the srun invocation; update the code in the same function/class that performs
the split (referencing the cmd variable and the later srun call) so you fail
fast with a helpful message like "sidecar command is empty" instead of allowing
an invalid srun invocation.
🧹 Nitpick comments (3)
configs/nat-eval/eval_config.yml (1)
108-108: Consider using a relative or configurable output directory.The absolute path
/logs/nat-eval-output/assumes a specific filesystem structure that may not exist on all systems or container environments. Consider using a relative path or an environment variable placeholder for portability.💡 Suggested alternatives
output: - dir: /logs/nat-eval-output/ + dir: ./logs/nat-eval-output/Or use an environment variable pattern if the config format supports it:
dir: ${NAT_OUTPUT_DIR:-./logs/nat-eval-output/}src/srtctl/cli/mixins/sidecar_stage.py (1)
133-136: Split the long preamble string to keep lines ≤120 chars.This line exceeds the 120‑character limit and will violate formatting rules. Consider splitting into shorter concatenated parts.
♻️ Proposed refactor
- parts.append( - f"echo 'Running setup script: {script_path}' && " - f"if [ -f '{script_path}' ]; then bash '{script_path}'; else echo 'WARNING: {script_path} not found'; fi" - ) + parts.append( + ( + "echo 'Running setup script: {script_path}' && " + "if [ -f '{script_path}' ]; then " + "bash '{script_path}'; " + "else echo 'WARNING: {script_path} not found'; fi" + ).format(script_path=script_path) + )As per coding guidelines, Line length must not exceed 120 characters.
recipes/nat/thompson.yaml (1)
71-78: Pin the NeMo-Agent-Toolkit clone to a tag for reproducibility.
Cloning the default branch makes benchmark runs non-deterministic over time. Use v1.4.0 (the latest stable release) or a specific commit from the GitHub releases page.♻️ Example pin (use v1.4.0 or another tagged release)
- git clone --depth 1 https://github.com/NVIDIA/NeMo-Agent-Toolkit.git /workspace/NeMo-Agent-Toolkit && + git clone --depth 1 --branch v1.4.0 https://github.com/NVIDIA/NeMo-Agent-Toolkit.git /workspace/NeMo-Agent-Toolkit &&
| function_groups: | ||
| banking_tools: | ||
| _type: banking_tools_group | ||
| tools_json_path: ./examples/dynamo_integration/data/raw/banking/tools.json |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the referenced tools JSON file exists
find . -path "*/examples/dynamo_integration/data/raw/banking/tools.json" -type f 2>/dev/null
# Also search more broadly for tools.json files
echo "=== Broader search for tools.json ==="
find . -name "tools.json" -type f 2>/dev/nullRepository: ishandhanani/srt-slurm
Length of output: 103
Add the missing tools.json file or update the path reference.
The tools JSON file at ./examples/dynamo_integration/data/raw/banking/tools.json does not exist in the repository. This will cause runtime failures during evaluation. Either add the missing file or correct the path reference in tools_json_path (line 15).
🤖 Prompt for AI Agents
In `@configs/nat-eval/eval_config.yml` at line 15, The path specified by the
tools_json_path setting in eval_config.yml points to a non-existent file
('./examples/dynamo_integration/data/raw/banking/tools.json'); either add the
missing tools.json at that location or update the tools_json_path value to the
correct relative path for the existing tools file so evaluations can load it;
locate the tools_json_path key in eval_config.yml and modify it or add the
referenced tools.json resource accordingly.
| dataset: | ||
| _type: json | ||
| file_path: ./examples/dynamo_integration/data/agent_leaderboard_v2_test_subset.json | ||
| structure: | ||
| disable: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "agent_leaderboard" --full-path || echo "Dataset file not found"Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check if the directory exists and list its contents
if [ -d "./examples/dynamo_integration/data/" ]; then
echo "Directory exists. Contents:"
ls -la "./examples/dynamo_integration/data/"
else
echo "Directory does not exist"
fi
# Also check if the specific file exists
if [ -f "./examples/dynamo_integration/data/agent_leaderboard_v2_test_subset.json" ]; then
echo "File exists"
else
echo "File does not exist"
fiRepository: ishandhanani/srt-slurm
Length of output: 110
🏁 Script executed:
# Check what actually exists in the examples directory
if [ -d "./examples" ]; then
echo "Examples directory exists. Structure:"
find ./examples -type d | head -20
echo ""
echo "Files in examples:"
find ./examples -type f | head -20
else
echo "Examples directory does not exist"
fiRepository: ishandhanani/srt-slurm
Length of output: 221
Dataset file does not exist. The path ./examples/dynamo_integration/data/agent_leaderboard_v2_test_subset.json referenced in the config does not exist in the repository. The ./examples directory contains only .yaml files with no dynamo_integration subdirectory. Either provide the missing dataset file or correct the file path.
🤖 Prompt for AI Agents
In `@configs/nat-eval/eval_config.yml` around lines 113 - 117, The dataset file
path in the eval config is invalid: update the dataset.file_path (or replace the
dataset block) to point to an existing file or add the missing JSON into the
repo; specifically edit the dataset entry (dataset._type, dataset.file_path,
dataset.structure) in eval_config.yml to either reference an existing example
file under ./examples (or correct subdirectory) or add the missing
./examples/dynamo_integration/data/agent_leaderboard_v2_test_subset.json file so
the path resolves.
| # Command is required for custom frontend (validated in schema) | ||
| assert fe_config.command is not None, "Custom frontend requires 'command' field" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/srtctl/cli/mixins/frontend_stage.pyRepository: ishandhanani/srt-slurm
Length of output: 109
🏁 Script executed:
sed -n '220,260p' src/srtctl/cli/mixins/frontend_stage.pyRepository: ishandhanani/srt-slurm
Length of output: 1624
🏁 Script executed:
# Search for the fe_config definition and its schema
rg -n "fe_config" src/srtctl/cli/mixins/frontend_stage.py | head -20Repository: ishandhanani/srt-slurm
Length of output: 666
🏁 Script executed:
# Look for schema definitions related to frontend
rg -n "command" src/srtctl/cli/mixins/frontend_stage.py | head -20Repository: ishandhanani/srt-slurm
Length of output: 697
🏁 Script executed:
# Find the config definition - likely imported from another module
head -50 src/srtctl/cli/mixins/frontend_stage.pyRepository: ishandhanani/srt-slurm
Length of output: 1586
🏁 Script executed:
# Search for frontend config schema or class definition
rg -n "class.*Frontend" src/srtctl --type pyRepository: ishandhanani/srt-slurm
Length of output: 431
🏁 Script executed:
# Look for dataclass or schema related to frontend
rg -n "frontend" src/srtctl/config --type py -iRepository: ishandhanani/srt-slurm
Length of output: 123
🏁 Script executed:
sed -n '878,950p' src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 2433
🏁 Script executed:
# Look for broader context around FrontendConfig
sed -n '860,920p' src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 2118
🏁 Script executed:
# Search for Schema definition for FrontendConfig
rg -n "class.*FrontendConfig.*Schema" src/srtctl --type pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look for schema validation with command and type="custom"
rg -n "command\|custom\|FrontendConfig" src/srtctl/core/schema.py | grep -i "schema\|valid"Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check if there's a separate schema file or validation logic
find src/srtctl -name "*.py" -type f -exec grep -l "FrontendConfig.*Schema\|marshmallow.*custom\|validates.*command" {} \;Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look at the full FrontendConfig class and see if there are field validators
rg -B5 -A30 "@dataclass.*frozen.*True.*\nclass FrontendConfig" src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 257
🏁 Script executed:
# Look for the entire FrontendConfig class definition including Schema
sed -n '878,925p' src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 1657
🏁 Script executed:
# Search for Schema definitions and post_load validators
rg -n "post_load\|validates\|@validates_schema" src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look for where FrontendConfig is used to see if validation happens at config load time
rg -B3 -A3 "FrontendConfig\|custom.*frontend" src/srtctl/cli/mixins/frontend_stage.py | head -40Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Search for where config is loaded and validated
rg -n "load_config\|SrtConfig\|from_json" src/srtctl --type py | head -20Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look for validation that happens when type="custom"
rg -B5 -A5 "type.*custom\|custom.*frontend" src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check the actual flow in frontend_stage.py - when is this called and what is fe_config state
sed -n '210,245p' src/srtctl/cli/mixins/frontend_stage.pyRepository: ishandhanani/srt-slurm
Length of output: 1484
🏁 Script executed:
# Search for Schema class definitions related to FrontendConfig
rg -n "class.*Schema" src/srtctl/core/schema.py | head -30Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check if marshmallow_dataclass is used and where validators are defined
rg -n "marshmallow_dataclass\|@post_load\|@validates" src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look for where config is validated when loaded
rg -n "def.*load\|from_json\|from_config" src/srtctl --type py | grep -i "config\|load"Repository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check if command field is validated conditionally based on type
rg -B10 -A10 "type.*=.*custom\|custom.*type" src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look for where _start_custom_frontend is called
rg -n "_start_custom_frontend" src/srtctl --type pyRepository: ishandhanani/srt-slurm
Length of output: 303
🏁 Script executed:
# Check the context where this method is called
sed -n '150,240p' src/srtctl/cli/mixins/frontend_stage.py | grep -B20 "_start_custom_frontend"Repository: ishandhanani/srt-slurm
Length of output: 1467
🏁 Script executed:
# Look at the entire _start_frontend method to see the flow
sed -n '70,170p' src/srtctl/cli/mixins/frontend_stage.pyRepository: ishandhanani/srt-slurm
Length of output: 3887
🏁 Script executed:
# Search for any validation code related to frontend config
rg -n "validate\|Validate" src/srtctl/core --type pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Check if there's a validator somewhere that checks type=="custom" requires command
rg -n "type.*custom\|custom.*command" src/srtctl --type pyRepository: ishandhanani/srt-slurm
Length of output: 48
🏁 Script executed:
# Look at the imports in schema.py to understand what schema/validation framework is used
head -100 src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 2709
🏁 Script executed:
# Check if there are any `@post_load` validators or conditional field validators in the entire schema.py
sed -n '1,100p' src/srtctl/core/schema.py | grep -i "post_load\|validates\|@"Repository: ishandhanani/srt-slurm
Length of output: 113
🏁 Script executed:
# Look for the full extent of FrontendConfig definition - any nested validators or field definitions
rg -A50 "^class FrontendConfig:" src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 1738
🏁 Script executed:
# Check if there are validator functions defined after the dataclass
sed -n '878,950p' src/srtctl/core/schema.pyRepository: ishandhanani/srt-slurm
Length of output: 2433
Replace assert with explicit exception for runtime validation.
Asserts can be disabled with the -O flag, leaving fe_config.command unchecked at runtime. Although the docstring indicates this field is required for custom frontends, there is no schema-level validation enforcing it. Use an explicit exception instead.
🧯 Proposed fix
- assert fe_config.command is not None, "Custom frontend requires 'command' field"
+ if fe_config.command is None:
+ raise ValueError("Custom frontend requires 'command' field")🤖 Prompt for AI Agents
In `@src/srtctl/cli/mixins/frontend_stage.py` around lines 237 - 239, Replace the
runtime assertion with an explicit exception: instead of using "assert
fe_config.command is not None", raise a clear exception (e.g., ValueError or
RuntimeError) when fe_config.command is missing so the check cannot be bypassed
with -O; update the check in the frontend stage where fe_config and its command
field are validated (the spot that currently reads the assert) to raise the
exception with the message "Custom frontend requires 'command' field".
| f"echo 'Running setup script: {script_path}' && " | ||
| f"if [ -f '{script_path}' ]; then bash '{script_path}'; else echo 'WARNING: {script_path} not found'; fi" | ||
| ) |
There was a problem hiding this comment.
Break the long command string to respect line length.
This line exceeds the 120-char limit and will be flagged by Ruff.
✂️ Suggested wrap
- parts.append(
- f"echo 'Running setup script: {script_path}' && "
- f"if [ -f '{script_path}' ]; then bash '{script_path}'; else echo 'WARNING: {script_path} not found'; fi"
- )
+ parts.append(
+ f"echo 'Running setup script: {script_path}' && "
+ f"if [ -f '{script_path}' ]; then "
+ f"bash '{script_path}'; "
+ f"else echo 'WARNING: {script_path} not found'; "
+ "fi"
+ )🤖 Prompt for AI Agents
In `@src/srtctl/cli/mixins/frontend_stage.py` around lines 294 - 296, The long
f-string that builds the shell command using script_path exceeds 120 characters;
split it into multiple shorter string parts (using parentheses for implicit
concatenation or '+' concatenation) so each line stays under 120 chars, e.g.
separate the "echo 'Running setup script: {script_path}' && " segment from the
"if [ -f '{script_path}' ]; then bash '{script_path}'; else echo 'WARNING:
{script_path} not found'; fi" segment and reassemble them into the same variable
(preserving the f-string interpolation for script_path).
| def start_sidecars(self) -> dict[str, ManagedProcess]: | ||
| """Start all configured sidecar processes. | ||
|
|
||
| Returns: | ||
| Dict mapping sidecar names to ManagedProcess instances. | ||
| """ | ||
| if not self.config.sidecars: | ||
| logger.debug("No sidecars configured") | ||
| return {} | ||
|
|
||
| logger.info("Starting %d sidecar(s)", len(self.config.sidecars)) | ||
| processes: dict[str, ManagedProcess] = {} | ||
|
|
||
| for name, sidecar_config in self.config.sidecars.items(): | ||
| proc = self._start_sidecar(name, sidecar_config) | ||
| processes[proc.name] = proc | ||
|
|
There was a problem hiding this comment.
Return-map keys don’t match stated sidecar names.
The docstring says the map is keyed by sidecar name, but the code uses proc.name (sidecar_{name}), which will break lookups by config key. Use the original name as the key (or update docstring/callers consistently).
🔧 Proposed fix
for name, sidecar_config in self.config.sidecars.items():
proc = self._start_sidecar(name, sidecar_config)
- processes[proc.name] = proc
+ processes[name] = proc📝 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.
| def start_sidecars(self) -> dict[str, ManagedProcess]: | |
| """Start all configured sidecar processes. | |
| Returns: | |
| Dict mapping sidecar names to ManagedProcess instances. | |
| """ | |
| if not self.config.sidecars: | |
| logger.debug("No sidecars configured") | |
| return {} | |
| logger.info("Starting %d sidecar(s)", len(self.config.sidecars)) | |
| processes: dict[str, ManagedProcess] = {} | |
| for name, sidecar_config in self.config.sidecars.items(): | |
| proc = self._start_sidecar(name, sidecar_config) | |
| processes[proc.name] = proc | |
| def start_sidecars(self) -> dict[str, ManagedProcess]: | |
| """Start all configured sidecar processes. | |
| Returns: | |
| Dict mapping sidecar names to ManagedProcess instances. | |
| """ | |
| if not self.config.sidecars: | |
| logger.debug("No sidecars configured") | |
| return {} | |
| logger.info("Starting %d sidecar(s)", len(self.config.sidecars)) | |
| processes: dict[str, ManagedProcess] = {} | |
| for name, sidecar_config in self.config.sidecars.items(): | |
| proc = self._start_sidecar(name, sidecar_config) | |
| processes[name] = proc |
🤖 Prompt for AI Agents
In `@src/srtctl/cli/mixins/sidecar_stage.py` around lines 45 - 61, The return map
from start_sidecars uses proc.name (which becomes "sidecar_{name}") as keys but
the docstring and callers expect the original sidecar config keys; change the
dictionary insertion in start_sidecars to use the loop variable name as the key
(processes[name] = proc) so keys match config names (or, if you prefer the
prefixed proc.name, update the docstring and all callers to expect that new key
format); key symbols: start_sidecars, _start_sidecar, processes, proc.name.
| # Parse user command into list | ||
| cmd = shlex.split(config.command) | ||
|
|
There was a problem hiding this comment.
Guard against empty sidecar commands.
shlex.split can return an empty list for blank/whitespace commands, which leads to an invalid srun invocation. Raise a clear error early.
🛡️ Proposed fix
- cmd = shlex.split(config.command)
+ cmd = shlex.split(config.command)
+ if not cmd:
+ raise ValueError(f"Sidecar '{name}' command is empty")🤖 Prompt for AI Agents
In `@src/srtctl/cli/mixins/sidecar_stage.py` around lines 93 - 95, After splitting
the sidecar command with shlex.split (the line setting cmd =
shlex.split(config.command)), guard against empty or whitespace-only commands by
checking if cmd is an empty list and raise a clear, early error (e.g.,
ValueError or a CLI-specific exception) before proceeding to the srun
invocation; update the code in the same function/class that performs the split
(referencing the cmd variable and the later srun call) so you fail fast with a
helpful message like "sidecar command is empty" instead of allowing an invalid
srun invocation.
1 participant
Summary by CodeRabbit
Release Notes
New Features
srtctl container-pullcommand for downloading container images defined in configurationDocumentation