feat: validate LAMMPS template revision variables before task execution#366
feat: validate LAMMPS template revision variables before task execution#366SchrodingersCattt wants to merge 3 commits into
Conversation
Add pre-check logic in LmpTemplateTaskGroup.make_task() that scans substituted templates for unreplaced V_* revision placeholders. This catches undefined revision variables at submit time (fail fast) instead of waiting until LAMMPS execution on remote cluster fails, which can waste hours of GPU training + queue time. Changes: - Add find_unreplaced_variables() to detect residual V_* patterns - Add check_revisions_completeness() with two checks: 1. Post-substitution residual check (raises ValueError) 2. Unused revision key detection (emits warning for typos) - Update test_lmp_empty to expect ValueError (previously would silently pass templates with unreplaced variables to LAMMPS) - Add TestRevisionVariablePrecheck test class with 5 test cases Closes: template variable typo wastes 75min train + queue time issue
dosubot
Bot
added
size:M
for more information, see https://pre-commit.ci
dosubot
Bot
added
size:L
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLAMMPS template task creation now detects unresolved ChangesRevision Placeholder Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🧹 Nitpick comments (1)
dpgen2/exploration/task/lmp_template_task_group.py (1)
314-320: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider using word boundaries for unused key detection.
Checking
if key not in template_rawuses simple substring matching. IfrevisionsdefinesV_NSTEPS, but the template only uses a longer variable likeV_NSTEPS_1, the substring match will still evaluate toTrue, inadvertently suppressing the unused key warning forV_NSTEPS.Since this is just a warning, it's not critical, but leveraging word boundaries ensures accurate matching.
💡 Proposed fix using regular expressions
# Check 2: Unused revision keys (warning only) if template_raw and revision_keys: for key in revision_keys: - if key not in template_raw: + if not re.search(rf"(?<![A-Za-z0-9_]){re.escape(key)}(?![A-Za-z0-9_])", template_raw): warnings.warn( f"Revision key '{key}' is defined but does not appear in the "🤖 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 `@dpgen2/exploration/task/lmp_template_task_group.py` around lines 314 - 320, Update the unused revision-key check in the loop over revision_keys to match complete variable names rather than raw substrings in template_raw. Use a word-boundary-aware regular-expression search so a key such as V_NSTEPS does not match V_NSTEPS_1, while preserving the existing warning behavior for genuinely absent keys.
🤖 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 `@dpgen2/exploration/task/lmp_template_task_group.py`:
- Around line 107-116: Update the check_revisions_completeness call in the
revisions validation block to validate every substituted template in conts, not
only conts[0]. Flatten or otherwise combine conts before passing it to the check
so PLUMED templates in conts[1] are checked whenever self.plm_set is enabled,
while preserving the existing revision keys and template_raw arguments.
---
Nitpick comments:
In `@dpgen2/exploration/task/lmp_template_task_group.py`:
- Around line 314-320: Update the unused revision-key check in the loop over
revision_keys to match complete variable names rather than raw substrings in
template_raw. Use a word-boundary-aware regular-expression search so a key such
as V_NSTEPS does not match V_NSTEPS_1, while preserving the existing warning
behavior for genuinely absent keys.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea5e019e-b0fd-4dba-9673-28774b4ebd37
📒 Files selected for processing (2)
dpgen2/exploration/task/lmp_template_task_group.pytests/exploration/test_lmp_templ_task_group.py
Address CodeRabbit review: check_revisions_completeness was only called with conts[0] (LAMMPS templates), missing conts[1] (PLUMED templates). Now flatten all template variants before validation so V_* placeholders in PLUMED templates are also caught. Add test_plumed_template_undefined_variable_raises test case.
1 participant
Problem
template.lammpsmay contain revision placeholders likeV_PRESSthat are not defined in therevisionsconfig dict. Currently this is only discovered when LAMMPS actually runs on the remote cluster and fails — potentially wasting hours of GPU training time plus queue wait.Solution
Add pre-check logic in
LmpTemplateTaskGroup.make_task()that validates revision variable substitution before task submission (fail fast):Post-substitution residual check: After applying
revise_by_keys(), scan output templates for remainingV_[A-Z][A-Z0-9_]*patterns. If found, raiseValueErrorwith a clear message listing undefined variables and available revisions.Unused key warning: If a revision key is defined but never appears in the template, emit
warnings.warn()to catch typos.Empty revisions with V_ in template*: If no revisions provided but template contains
V_*variables, raiseValueError.Design decisions
V_prefix is a strong convention from dpgen v1/v2 (100% of tests/examples use it). The check only scans for this pattern.${VARNAME}syntax is correctly ignored.Tests
TestRevisionVariablePrechecktest_lmp_emptyto expectValueErrorAll 9 tests pass.
Summary by CodeRabbit