Skip to content

Commit 34fbe89

Browse files
committed
fix: address review findings — double walk, N captures, short prefix, bash doc
- validate_local_path: single directory walk instead of two rglob passes - srtctl check: capture fingerprint once, reuse for all worker comparisons - model.revision: require >= 7 chars to prevent false prefix matches - Document bash requirement for process substitution in heredoc script
1 parent 080577e commit 34fbe89

3 files changed

Lines changed: 21 additions & 8 deletions

File tree

src/srtctl/cli/submit.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,13 @@
4141
load_config,
4242
resolve_config_with_defaults,
4343
)
44-
from srtctl.core.fingerprint import check_against_fingerprint, diff_fingerprints, format_check_results, format_diff
44+
from srtctl.core.fingerprint import (
45+
capture_fingerprint,
46+
check_against_fingerprint,
47+
diff_fingerprints,
48+
format_check_results,
49+
format_diff,
50+
)
4551
from srtctl.core.lockfile import load_lockfile_fingerprints
4652
from srtctl.core.schema import SrtConfig
4753
from srtctl.core.status import create_job_record
@@ -1053,10 +1059,11 @@ def add_common_args(p):
10531059
console.print(f"[bold red]Could not load fingerprints from:[/] {args.path}")
10541060
sys.exit(1)
10551061

1056-
# Check each worker's fingerprint against current environment
1062+
# Capture current environment once, reuse for all worker checks
1063+
current_fp = capture_fingerprint()
10571064
all_results = []
10581065
for worker in sorted(fps.keys()):
1059-
results = check_against_fingerprint(fps[worker])
1066+
results = check_against_fingerprint(fps[worker], current_fp)
10601067
if results:
10611068
all_results.extend(results)
10621069
console.print(f"\n[bold]{worker}:[/]")

src/srtctl/core/fingerprint.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ def verify_identity(
647647
)
648648
)
649649

650-
if identity.model.revision:
650+
if identity.model.revision and len(identity.model.revision) >= 7:
651651
fp_rev = fp_model.get("hf_revision")
652652
if fp_rev and fp_rev.startswith(identity.model.revision):
653653
results.append(IdentityCheckResult("model.revision", True, f"{fp_rev[:12]}"))
@@ -878,8 +878,10 @@ def env_vars():
878878
Path('{output_path}').parent.mkdir(parents=True, exist_ok=True)
879879
Path('{output_path}').write_text(json.dumps(fp, indent=2) + '\\n')
880880
"""
881-
# Use a heredoc to write the script to a temp file, then execute it.
881+
# Use a heredoc via process substitution to pass the script to python3.
882882
# This is immune to quoting/escaping issues in the bash → srun chain.
883+
# NOTE: <(...) requires bash (not POSIX sh/dash). srun containers use bash.
884+
# The || true suffix means non-bash shells silently skip fingerprinting.
883885
return f"python3 <(cat <<'__FINGERPRINT_EOF__'\n{script}__FINGERPRINT_EOF__\n) || true"
884886

885887

src/srtctl/core/validation.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,13 @@ def validate_local_path(name: str, path: str) -> ValidationResult:
4343
if not p.exists():
4444
return ValidationResult(name, False, f"not found: {path}")
4545
if p.is_dir():
46-
file_count = sum(1 for _ in p.rglob("*") if _.is_file())
47-
total_gb = sum(f.stat().st_size for f in p.rglob("*") if f.is_file()) / 1e9
48-
return ValidationResult(name, True, f"{file_count} files, {total_gb:.1f}GB")
46+
file_count = 0
47+
total_bytes = 0
48+
for f in p.rglob("*"):
49+
if f.is_file():
50+
file_count += 1
51+
total_bytes += f.stat().st_size
52+
return ValidationResult(name, True, f"{file_count} files, {total_bytes / 1e9:.1f}GB")
4953
size_gb = p.stat().st_size / 1e9
5054
return ValidationResult(name, True, f"{size_gb:.1f}GB")
5155
except Exception as e:

0 commit comments

Comments
 (0)