Add standalone checkpoint evaluation for Puzzletron - #2177
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a reusable ChangesPuzzletron evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The evaluator now persists a result path that may refer to different output artifacts, which could cause campaign consumers to read the wrong file. The PR is mergeable with explicit owner awareness and follow-up to clarify or normalize this contract. Sequence Diagram(s)sequenceDiagram
participant CLI
participant run_lmms_eval_checkpoint
participant lmms_eval
participant AttemptDirectory
CLI->>run_lmms_eval_checkpoint: pass checkpoint and evaluation settings
run_lmms_eval_checkpoint->>lmms_eval: run generated vLLM command
lmms_eval-->>run_lmms_eval_checkpoint: return output and result JSON
run_lmms_eval_checkpoint->>AttemptDirectory: write command, streams, summary, and metrics
run_lmms_eval_checkpoint-->>CLI: return normalized JSON result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 4
🧹 Nitpick comments (2)
examples/puzzletron/evaluate_lmms_checkpoint.py (1)
271-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
__all__and a docstring formain.
mainis the public entry point of this example module. Declare the public surface and document the arguments, return code, and stdout/stderr contract.♻️ Proposed change
+__all__ = ["main"] + + def main(argv: list[str] | None = None) -> int: + """Evaluate one local checkpoint and print machine-readable JSON to stdout. + + Args: + argv: Command-line arguments; defaults to `sys.argv[1:]`. + + Returns: + 0 on success, 1 on failure. Failure diagnostics go to stderr as JSON. + """ args = _build_parser().parse_args(argv)Place
__all__near the module constants.As per coding guidelines: "Define each module's public API with
__all__" and "Document public and higher-level APIs with docstrings, including examples when useful."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/puzzletron/evaluate_lmms_checkpoint.py` around lines 271 - 272, Add a module-level __all__ near the existing constants that exposes main, and add a docstring to main documenting the optional argv argument, integer return code, and stdout/stderr behavior while preserving its current argument parsing and execution flow.Source: Coding guidelines
tests/unit/torch/puzzletron/test_lmms_evaluation.py (1)
275-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one end-to-end case that runs the real
_run_process.Every
run_lmms_eval_checkpointtest replaces_run_processwith a fake, so the realPopenpath, cwd handling, and stream capture stay untested. A hermetic case is cheap: setcommand_prefixto[sys.executable, "-c", script]wherescriptwrites aresults.jsoninto the current directory and exits 0. The existing fake-based tests can stay for the failure paths.As per coding guidelines: "Tests must exercise the behavior they claim to validate. For backend or runtime behavior, include an end-to-end test using the real implementation rather than replacing it with a fake."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/torch/puzzletron/test_lmms_evaluation.py` around lines 275 - 311, Add an end-to-end test for run_lmms_eval_checkpoint that does not monkeypatch lmms._run_process. Configure command_prefix with sys.executable and an inline script that writes results.json in the current working directory and exits successfully, then assert the returned metrics and captured command, stdout, stderr, and result artifacts. Keep the existing fake-based tests for failure-path coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modelopt/torch/puzzletron/evaluation/lmms.py`:
- Line 25: Remove the inline `# nosec B404` from the `subprocess` import at
modelopt/torch/puzzletron/evaluation/lmms.py:25-25 and remove `# nosec B603`
from the `subprocess.Popen` call at
modelopt/torch/puzzletron/evaluation/lmms.py:535-536, preserving the explanatory
comment at line 535. If Bandit still requires an exception, configure it in
repository settings or obtain the specified codeowner approval with PR
justification.
- Around line 230-242: Update _command_prefix to tokenize string command_prefix
values with shlex.split, matching the existing _extra_args behavior, while
preserving sequence handling and the current empty-value validation. Ensure a
string such as “python -m lmms_eval” produces separate argv elements rather than
one executable name.
- Around line 323-337: Update the environment construction in the settings
normalization flow so an explicit settings["cache_dir"] sets LMMS_EVAL_HOME even
when the process environment already contains that variable, while preserving a
caller-provided settings["env"]["LMMS_EVAL_HOME"] override. Replace the
setdefault behavior around cache_dir without changing the timeout handling.
In `@modelopt/torch/puzzletron/post_mip/runner.py`:
- Line 24: Remove the # nosec B404 suppression from the subprocess import in
runner.py. Import TimeoutExpired directly, then update the exception checks in
the runner logic around the two timeout-handling usages to reference
TimeoutExpired while preserving the existing TimeoutError behavior.
---
Nitpick comments:
In `@examples/puzzletron/evaluate_lmms_checkpoint.py`:
- Around line 271-272: Add a module-level __all__ near the existing constants
that exposes main, and add a docstring to main documenting the optional argv
argument, integer return code, and stdout/stderr behavior while preserving its
current argument parsing and execution flow.
In `@tests/unit/torch/puzzletron/test_lmms_evaluation.py`:
- Around line 275-311: Add an end-to-end test for run_lmms_eval_checkpoint that
does not monkeypatch lmms._run_process. Configure command_prefix with
sys.executable and an inline script that writes results.json in the current
working directory and exits successfully, then assert the returned metrics and
captured command, stdout, stderr, and result artifacts. Keep the existing
fake-based tests for failure-path coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ca1dc7a-3932-43a7-adbd-bb3b1b633da4
📒 Files selected for processing (13)
examples/puzzletron/README.mdexamples/puzzletron/ci_environment.jsonexamples/puzzletron/docs/checkpoint_evaluation.mdexamples/puzzletron/docs/post_mip_pipeline.mdexamples/puzzletron/evaluate_lmms_checkpoint.pyexamples/puzzletron/requirements.txtmodelopt/torch/puzzletron/__init__.pymodelopt/torch/puzzletron/evaluation/__init__.pymodelopt/torch/puzzletron/evaluation/lmms.pymodelopt/torch/puzzletron/post_mip/runner.pytests/unit/torch/puzzletron/test_evaluate_lmms_checkpoint_cli.pytests/unit/torch/puzzletron/test_lmms_evaluation.pytests/unit/torch/puzzletron/test_post_mip_runner.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/puzzletron_v2 #2177 +/- ##
=========================================================
+ Coverage 53.15% 53.52% +0.36%
=========================================================
Files 704 706 +2
Lines 91506 91565 +59
=========================================================
+ Hits 48640 49009 +369
+ Misses 42866 42556 -310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
22317fb to
ec0f281
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🧹 Nitpick comments (2)
modelopt/torch/puzzletron/evaluation/lmms.py (2)
273-280: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExclude
bytesandbytearrayin theextra_argssequence branch.
_command_prefixat Line 260 rejectsbytesandbytearraybefore the sequence branch._extra_argsdoes not. Abytesvalue iterates as integers, sovaluesbecomes a list of decimal byte codes and the argv is silently wrong. Align both helpers.♻️ Proposed change
if isinstance(raw, str): values = shlex.split(raw) - elif isinstance(raw, Sequence): + elif isinstance(raw, Sequence) and not isinstance(raw, (bytes, bytearray)): values = [str(item) for item in raw] else: raise TypeError("evaluation settings.extra_args must be a string or sequence")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/torch/puzzletron/evaluation/lmms.py` around lines 273 - 280, Update the extra_args parsing branch in _extra_args to exclude bytes and bytearray from the Sequence check, matching the validation performed by _command_prefix. Ensure these values are rejected rather than converted into integer byte codes, while preserving handling for strings and other valid sequences.
701-714: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGive the two
result_pathvalues distinct names.
summary.jsonstoresresult_pathas the raw lmms-eval JSON path at Line 704. The returned mapping storesresult_pathas the summary path at Line 710 and the raw path underraw_result_path. The same key therefore means two different files depending on where a consumer reads it._downstream_evaluationinmodelopt/torch/puzzletron/post_mip/runner.pyreturns this mapping directly into candidate rows, so the ambiguity reaches persisted campaign records.Rename the key inside the summary payload, for example to
raw_result_path, so both surfaces agree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelopt/torch/puzzletron/evaluation/lmms.py` around lines 701 - 714, Rename the summary payload’s `result_path` field in the `summary` object to `raw_result_path`, matching the returned mapping and ensuring both surfaces identify the raw lmms-eval JSON consistently. Leave the returned `result_path` pointing to `summary_path` and preserve the existing `raw_result_path` entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/puzzletron/evaluate_lmms_checkpoint.py`:
- Around line 219-224: Update the argument parser near the existing
trust-remote-code option to accept a remainder argument such as
--lmms-eval-args, allowing native lmms-eval options without parser rejection. In
_settings(), merge those forwarded values with the compatibility --include_path
entry in settings["extra_args"], preserving include_path behavior. Add a CLI
test that verifies an allowed native option is forwarded, while leaving backend
rejection of reserved flags unchanged.
In `@modelopt/torch/puzzletron/evaluation/lmms.py`:
- Around line 574-585: Update the process-wait timeout handling around the
existing asyncio.wait_for calls to catch both asyncio.TimeoutError and the
built-in TimeoutError. Preserve the current SIGTERM/SIGKILL cleanup flow and
ensure the outer timeout path still classifies the run as timed_out.
---
Nitpick comments:
In `@modelopt/torch/puzzletron/evaluation/lmms.py`:
- Around line 273-280: Update the extra_args parsing branch in _extra_args to
exclude bytes and bytearray from the Sequence check, matching the validation
performed by _command_prefix. Ensure these values are rejected rather than
converted into integer byte codes, while preserving handling for strings and
other valid sequences.
- Around line 701-714: Rename the summary payload’s `result_path` field in the
`summary` object to `raw_result_path`, matching the returned mapping and
ensuring both surfaces identify the raw lmms-eval JSON consistently. Leave the
returned `result_path` pointing to `summary_path` and preserve the existing
`raw_result_path` entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b1527127-9bcd-4349-ba49-ade485d0367a
📒 Files selected for processing (5)
examples/puzzletron/evaluate_lmms_checkpoint.pymodelopt/torch/puzzletron/evaluation/lmms.pymodelopt/torch/puzzletron/post_mip/runner.pytests/unit/torch/puzzletron/test_evaluate_lmms_checkpoint_cli.pytests/unit/torch/puzzletron/test_lmms_evaluation.py
Forward native lmms-eval options through the convenience CLI, preserve timeout compatibility, reject byte-string arguments, and disambiguate persisted result paths. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
1 participant
What does this PR do?
Type of change: New feature
Adds a standalone command for evaluating compatible local Hugging Face checkpoints with lmms-eval and vLLM, without creating or running a Puzzletron campaign. The reusable evaluator owns command construction, completion validation, timeout cleanup, and isolated attempt artifacts; downstream campaign evaluation now consumes that evaluator as an adapter.
The convenience command provides a small text smoke by default, detects Qwen 3.5 checkpoint metadata and supplies the required reasoning parser automatically, and exposes explicit override and opt-out controls. It keeps the native lmms-eval CLI available for options outside the convenience surface. The pinned evaluator snapshot and pass-through argument surfaces keep behavior reproducible without coupling ModelOpt to lmms-eval internals.
Usage
Use
--fullafter the smoke succeeds to run complete task datasets. Qwen 3.5 checkpoints are configured automatically.Testing
Adds unit coverage for the standalone CLI defaults, Qwen detection and overrides, command construction and reserved-argument protection, result completeness, artifact preservation, timeout cleanup, and downstream campaign reuse. Existing Puzzletron CI covers the pinned evaluator environment.
Summary by CodeRabbit
New Features
Improvements