Skip to content

Commit c4e6d35

Browse files
committed
review: harden fingerprint PR after code review
- IdentityCheckResult now frozen (consistency with other dataclasses) - Extract FRAMEWORK_PACKAGES constant (eliminates hardcoded duplicates) - Remove hasattr(config, 'identity') checks (field always exists via default_factory) - Reduce HF validation timeout 5s -> 2s (air-gapped clusters) - Reduce bash script probe timeout 5s -> 3s (faster worker startup) - Simplify find_python() (Path.exists() instead of subprocess)
1 parent a5d8477 commit c4e6d35

4 files changed

Lines changed: 22 additions & 27 deletions

File tree

src/srtctl/cli/mixins/benchmark_stage.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ def run_benchmark(
101101
self._identity_verification = None
102102
try:
103103
fingerprints = collect_worker_fingerprints(self.runtime.log_dir)
104-
if fingerprints and hasattr(self.config, "identity"):
104+
if fingerprints and self.config.identity:
105105
self._identity_verification = verify_identity(self.config.identity, fingerprints)
106106
banner = format_identity_verification(self._identity_verification, self.config.identity)
107107
for line in banner.splitlines():

src/srtctl/cli/submit.py

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -270,13 +270,9 @@ def _print_running_summary(config: SrtConfig, console: Console) -> None:
270270
console.print(f" Backend: {config.backend_type}")
271271
console.print(f" Benchmark: {config.benchmark.type}")
272272

273-
has_identity = (
274-
hasattr(config, "identity")
275-
and config.identity
276-
and (
277-
(config.identity.model and (config.identity.model.repo or config.identity.model.revision))
278-
or config.identity.frameworks
279-
)
273+
has_identity = config.identity and (
274+
(config.identity.model and (config.identity.model.repo or config.identity.model.revision))
275+
or config.identity.frameworks
280276
)
281277
if has_identity:
282278
id_fields = []
@@ -362,7 +358,7 @@ def submit_with_orchestrator(
362358
)
363359

364360
# Identity validation (inline, <1s) — runs for both dry-run and submit
365-
if hasattr(config, "identity") and config.identity and config.identity.model and config.identity.model.repo:
361+
if config.identity and config.identity.model and config.identity.model.repo:
366362
from srtctl.core.validation import validate_hf_model
367363

368364
hf_result = validate_hf_model(config.identity.model.repo, config.identity.model.revision)

src/srtctl/core/fingerprint.py

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,15 @@
4040
# Sentinel for probes that failed
4141
UNAVAILABLE = "unavailable"
4242

43+
# Framework name -> pip package name mapping.
44+
# Used in both native Python probes and the bash capture script.
45+
FRAMEWORK_PACKAGES: dict[str, str] = {
46+
"vllm": "vllm",
47+
"sglang": "sglang",
48+
"tensorrt_llm": "tensorrt-llm",
49+
"dynamo": "ai-dynamo",
50+
}
51+
4352

4453
# ============================================================================
4554
# Data Types
@@ -253,12 +262,7 @@ def probe_frameworks() -> ProbeResult:
253262
GPU context.
254263
"""
255264
versions: dict[str, str] = {}
256-
for name, pkg in [
257-
("vllm", "vllm"),
258-
("sglang", "sglang"),
259-
("tensorrt_llm", "tensorrt-llm"),
260-
("dynamo", "ai-dynamo"),
261-
]:
265+
for name, pkg in FRAMEWORK_PACKAGES.items():
262266
v = _run_cmd(f"python3 -c \"import importlib.metadata; print(importlib.metadata.version('{pkg}'))\"")
263267
if v:
264268
versions[name] = v
@@ -584,7 +588,7 @@ def check_against_fingerprint(
584588
# ============================================================================
585589

586590

587-
@dataclass
591+
@dataclass(frozen=True)
588592
class IdentityCheckResult:
589593
"""Result of a single identity check."""
590594

@@ -742,16 +746,16 @@ def generate_capture_script(output_path: str) -> str:
742746
from pathlib import Path
743747
from datetime import datetime, timezone
744748
745-
def run(cmd, timeout=5):
749+
def run(cmd, timeout=3):
746750
try:
747751
r = subprocess.run(cmd, shell=True, capture_output=True, text=True, timeout=timeout)
748752
return r.stdout.strip() if r.returncode == 0 else None
749753
except Exception:
750754
return None
751755
752756
def find_python():
753-
for p in ['/opt/dynamo/venv/bin/python3', '/opt/venv/bin/python3', 'python3']:
754-
if run(f'command -v {{0}} || test -x {{0}}'.format(p)):
757+
for p in ['/opt/dynamo/venv/bin/python3', '/opt/venv/bin/python3']:
758+
if Path(p).exists():
755759
return p
756760
return 'python3'
757761
@@ -786,12 +790,7 @@ def framework_versions():
786790
# Use importlib.metadata for all packages — avoids loading native CUDA
787791
# extensions (tensorrt_llm, torch) which fail without GPU context.
788792
versions = {{}}
789-
for name, pkg in [
790-
('vllm', 'vllm'),
791-
('sglang', 'sglang'),
792-
('tensorrt_llm', 'tensorrt-llm'),
793-
('dynamo', 'ai-dynamo'),
794-
]:
793+
for name, pkg in [{", ".join(f"('{n}', '{p}')" for n, p in FRAMEWORK_PACKAGES.items())}]:
795794
v = run(f"{{PY}} -c \\"import importlib.metadata; print(importlib.metadata.version('{{pkg}}'))\\"".format(PY=PY, pkg=pkg))
796795
if v:
797796
versions[name] = v

src/srtctl/core/validation.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
logger = logging.getLogger(__name__)
2626

27-
_HTTP_TIMEOUT = 5.0
27+
_HTTP_TIMEOUT = 2.0 # Fast enough for live networks, doesn't block long on air-gapped clusters
2828

2929

3030
@dataclass(frozen=True)
@@ -147,7 +147,7 @@ def run_all_validations(config: SrtConfig) -> list[ValidationResult]:
147147
try:
148148
hf_repo = None
149149
hf_rev = None
150-
if hasattr(config, "identity") and config.identity and config.identity.model:
150+
if config.identity and config.identity.model:
151151
hf_repo = config.identity.model.repo
152152
hf_rev = config.identity.model.revision
153153
results.append(validate_hf_model(hf_repo, hf_rev))

0 commit comments

Comments
 (0)