feat: validate LAMMPS template revision variables before task execution - #366
feat: validate LAMMPS template revision variables before task execution#366SchrodingersCattt wants to merge 19 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughLAMMPS and PLUMED template task creation now validates unresolved ChangesRevision Placeholder Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds pre-execution validation for revision placeholders and deterministic failure handling; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant make_task
participant check_revisions_completeness
participant Templates
participant Diagnostics
make_task->>check_revisions_completeness: validate raw and rendered templates
check_revisions_completeness->>Templates: inspect LAMMPS and PLUMED content
Templates-->>check_revisions_completeness: return unresolved or unused revision keys
check_revisions_completeness-->>Diagnostics: raise FatalError or emit warning
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title clearly describes the main change: validating LAMMPS template revision variables before task execution. It does not mention PLUMED templates, but this omission does not make the title misleading. Full details: Docstring CoverageExplanation Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 5 files. (1 skipped: 1 unsupported.)
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.
The ValueError for 'no revisions but template has V_*' broke existing tests (test_submit.TestSubmitCmdStd) that legitimately use templates with V_* placeholders without providing revisions (e.g., customized- lmp-template workflows where substitution is handled externally). Change to warnings.warn() instead of raise ValueError for the empty-revisions case. The hard ValueError is still raised when revisions ARE provided but incomplete (the important fail-fast case). Update tests to expect UserWarning instead of ValueError.
This reverts commit edd20b1.
Avoid false positives when V_* appears in comments (e.g., '# TODO: add V_PRESS support later'). The regex now only scans non-comment portions of each line. Add _strip_lammps_comments() helper and test_commented_variables_not_flagged.
| template_raw += "\n" + "\n".join(self.plm_template) | ||
| # Flatten all template variants (LAMMPS + PLUMED) for validation | ||
| all_conts = [c for c_list in conts for c in c_list] | ||
| check_revisions_completeness( |
There was a problem hiding this comment.
P1 — Validate placeholder names before substring substitution. This check runs only after make_cont() has called revise_by_keys(), which uses plain str.replace. If revisions defines V_TEMP but the template contains an undefined V_TEMPERATURE, substitution turns it into 300ERATURE; the residual regex then sees no V_* token, so the fail-fast feature silently misses the typo. Compare raw placeholder tokens against the revision-key set before substitution, and make replacement token-aware (or otherwise handle overlapping defined keys) so prefix collisions cannot destroy the evidence; add regressions for both an undefined longer token and two defined overlapping tokens. This needs coordinated changes across validation and substitution, so a single-line suggestion would be incomplete.
Codex quota is about to reset, so I am using the remaining token budget to review this PR now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| stripped = [] | ||
| for line in lines: | ||
| # LAMMPS comments start with # (not inside quotes for our purposes) | ||
| idx = line.find("#") |
There was a problem hiding this comment.
P2 — Preserve # characters inside quoted LAMMPS strings. LAMMPS only starts a comment at an unquoted #, but this strips at the first # unconditionally. For example, print "# target V_MISSING" is executable template content; this helper removes V_MISSING before scanning, so an undefined placeholder is not reported. Please use quote-aware comment stripping (covering both quote forms and escapes as supported by the template grammar) and add regression tests for quoted hashes. A safe patch requires a small parser rather than a localized one-line replacement.
Codex quota is about to reset, so I am using the remaining token budget to review this PR now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The fail-fast validation has two false-negative paths on the current head: raw substring substitution can erase an undefined longer placeholder before validation, and comment stripping incorrectly treats quoted # characters as comment starts. I left exact inline reproductions and the required test cases. The targeted test module could not be collected locally because the active environment lacks dflow; the reported pre-commit and documentation checks pass.
Codex quota is about to reset, so I am using the remaining token budget to review this PR now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dpgen2/exploration/task/lmp_template_task_group.py (1)
375-383: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse complete placeholder matching for unused revision keys.
Line 378 uses a substring check. A template that contains only
V_TEMPERATUREmakes configuredV_TEMPappear used. AV_TEMPoccurrence in a comment also suppresses the warning.Use
raw_variables, which already applies the required token and comment rules. Add a regression for configuredV_TEMPwith a raw template that contains onlyV_TEMPERATURE.Proposed fix
if template_raw and revision_keys: for key in revision_keys: - if key not in template_raw: + if key not in raw_variables: warnings.warn(🤖 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 375 - 383, Update the unused-key check in the revision-key validation block to compare each key against the parsed, comment-aware token set from raw_variables rather than using substring matching on template_raw. Preserve the existing warning behavior for truly unused keys, and add a regression covering configured V_TEMP with a raw template containing only V_TEMPERATURE.
🤖 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.
Outside diff comments:
In `@dpgen2/exploration/task/lmp_template_task_group.py`:
- Around line 375-383: Update the unused-key check in the revision-key
validation block to compare each key against the parsed, comment-aware token set
from raw_variables rather than using substring matching on template_raw.
Preserve the existing warning behavior for truly unused keys, and add a
regression covering configured V_TEMP with a raw template containing only
V_TEMPERATURE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7e9a52c-e55d-4243-99de-5ff815726bb7
📒 Files selected for processing (2)
dpgen2/exploration/task/lmp_template_task_group.pytests/exploration/test_lmp_templ_task_group.py
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Code review
The goal is right — catching a missing V_PRESS at submit time instead of after hours of queue wait is worth having. But most of the findings below collapse into one root cause, so please read them as one change rather than six patches.
Root cause: the check validates the substituted output instead of the raw template.
Scanning conts after revise_by_keys() has already run means the checker only sees what survived substitution. Deriving the answer from the template instead — roughly
found = find_unreplaced_variables(template_raw) # raw, not substituted
missing = found - set(revision_keys) # -> error
unused = set(revision_keys) - found # -> warningfixes items 1, 3 and the spurious-error case in one move, and makes the unused-key check (item 4) fall out for free instead of needing its own separate substring test.
Item 6 (cannot be anchored — the PR does not touch this file): the V_ convention is not the documented contract.
The declared behaviour of revisions is "Key is the word to be replaced in the templates" — any word, no prefix requirement:
dpgen2/dpgen2/exploration/task/make_task_group_from_config.py
Lines 134 to 136 in 44ddb10
So a user following the documented contract with revisions: {"MYTEMP": [300]} gets no protection at all from this check, while still being exposed to the false positive in item 3. If the V_* namespace is going to be reserved and enforced, doc_revisions and docs/input.md need to say so.
CI has not run on this PR. gh pr checks shows 3 passing checks; merged PR #343 had 10. Python unit-tests and Type checker are both conclusion=action_required on 44ddb10 — the outside-contributor gate. Nothing executed, so the green checkmarks are not evidence the suite passes. A maintainer needs to approve the workflow run before these findings (or the fix) can be judged against real test results.
| # Check 1: Residual unreplaced variables | ||
| all_unreplaced: Set[str] = set() | ||
| for content in templates_content: | ||
| all_unreplaced.update(find_unreplaced_variables(content)) |
There was a problem hiding this comment.
1. The check scans post-substitution output, so it misses the most common typo shape — and reports the template as clean.
revise_by_keys does a plain unanchored str.replace(key, value). If a defined key is a prefix of an undefined token, the replace eats it and leaves no V_* residue for this scan to find. Reproduced on this branch:
template: variable TEMP equal V_TEMPERATURE # typo, user meant V_TEMP
revisions: {"V_TEMP": [300]}
result: no ValueError, no warning
variable TEMP equal 300ERATURE # garbage, fails at LAMMPS runtime
Check 2 does not catch it either, because "V_TEMP" in "...V_TEMPERATURE..." is True.
Worse, the same mechanism corrupts a correctly declared key. With revisions = {"V_TEMP": [300], "V_TEMP_HI": [600]}, V_TEMP is substituted first and mangles the other placeholder into 300_HI — the intended 600 never lands, and neither check fires.
This is the failure mode the PR exists to prevent, and the check currently gives false assurance that it has been ruled out. Scanning the raw template and diffing against revisions.keys() catches both cases.
| all_unreplaced.update(find_unreplaced_variables(content)) | ||
|
|
||
| if all_unreplaced: | ||
| raise ValueError( |
There was a problem hiding this comment.
2. Bare ValueError inside a dflow step will be treated as retryable, defeating the fail-fast goal.
LmpTemplateTaskGroup.make_task() is not only called at submit time — it is re-invoked every iteration inside the running workflow: flow/dpgen_loop.py SchedulerWrapper.execute() -> scheduler.plan_next_iteration() -> ConvergenceCheckStageScheduler.plan_next_iteration() -> ExplorationStage.make_task() (stage.py:73-74) -> here.
Two consequences:
exploration/scheduler/scheduler.py:140-148only catchesFatalErrorand re-wraps it with stage context. AValueErrorescapes that, so the operator loses which stage failed.- dflow's generated OP script special-cases
TransientError -> exit 1andFatalError -> exit 2. An uncaughtValueErrorfalls through to Python's default uncaught-exception code, which is also 1 — indistinguishable fromTransientError. Aretry_on_transient_errorstrategy will therefore retry a deterministic config error until the retry budget is exhausted.
dflow.python.FatalError is the established convention in this exact call path — see customized_lmp_template_task_group.py:195 and convergence_check_stage_scheduler.py. Please raise that instead.
|
|
||
| # Regex pattern for dpgen-style revision placeholders: V_ followed by uppercase letters/digits/underscores. | ||
| # This matches the universal convention in dpgen v1/v2 (all tests, docs, and examples use V_XXX). | ||
| _REVISION_VARIABLE_PATTERN = re.compile( |
There was a problem hiding this comment.
3. Hard, unsuppressible ValueError on legal LAMMPS identifiers that begin with V_.
LAMMPS variable names are [A-Za-z0-9_]+, so V_MAX is a perfectly legal user variable. The lookbehind exempts the lowercase v_name dereference form, but not these:
variable V_MAX equal 3.0
velocity all create ${V_MAX} 1
Verified against this regex: both lines match, and with any non-empty revisions the workflow raises ValueError: ... undefined revision variable(s): ['V_MAX']. That template works today and becomes un-submittable after this PR, with no config flag or override to escape it. The population most likely to hit this is exactly the population that uses revisions — dpgen2's own convention pushes V_UPPERCASE naming through the whole template. Same applies to PLUMED LABEL=/FILE= values.
The PR description says "LAMMPS internal ${VARNAME} syntax is correctly ignored", but test_lammps_internal_variables_not_flagged only exercises ${TEMP} and ${NSTEPS}, which contain no V_ at all — the test never covers the claim it is named for. ${V_TEMP} is flagged.
Deriving the check from the raw template minus revisions.keys() removes this class of false positive entirely. If the output scan is kept, this needs at minimum an opt-out, given the runtime failure it replaces is already loud and self-diagnosing.
Minor, same line: the comment says "V_ followed by uppercase letters/digits/underscores", but the pattern requires a letter immediately after V_ — V_2FOO and V__FOO do not match.
| # Check 2: Unused revision keys (warning only) | ||
| if template_raw and revision_keys: | ||
| for key in revision_keys: | ||
| if key not in template_raw: |
There was a problem hiding this comment.
4. Comment stripping is applied asymmetrically between the two checks, so the unused-key warning is suppressed exactly when it is useful.
Check 1 runs its content through _strip_lammps_comments(). Check 2 here does a raw key not in template_raw against the unstripped template. A revision key whose only surviving occurrence is inside a commented-out line therefore counts as "used" and produces no warning:
template_raw = "variable NSTEPS equal V_NSTEPS\n# variable PRESS equal V_PRESS\n"
revisions = {"V_NSTEPS": [...], "V_PRESS": [...]}
# -> no warningCommenting out a block during iteration and forgetting to prune revisions is the exact scenario this warning is meant to catch — examples/chno/template.lammps already ships with commented-out fix lines, so the pattern is normal in this codebase. Strip comments in check 2 as well (or drop check 2 in favour of the raw-template set difference, which handles it inherently).
|
|
||
| This function performs two checks: | ||
| 1. **Post-substitution residual check**: After applying revisions, scan the output | ||
| for any remaining V_* variables that were not replaced. This catches typos in |
There was a problem hiding this comment.
5. Docstring overclaims what check 1 does.
"This catches typos in template variables or missing keys in revisions" is not true for the prefix-collision case in item 1 — V_TEMP declared against a V_TEMPERATURE typo passes both checks silently. Either fix the check so the docstring becomes true, or state the limitation here.
| self.assertGreater(len(typo_warnings), 0) | ||
|
|
||
| def test_lammps_internal_variables_not_flagged(self): | ||
| """${NSTEPS} and similar LAMMPS internal refs should NOT be flagged.""" |
There was a problem hiding this comment.
This docstring implies there is deliberate logic excluding LAMMPS ${VAR} dereference syntax. There is not — the exclusion is incidental to the regex requiring a literal V_ prefix, and ${V_TEMP} (legal LAMMPS) is flagged and raises. The test passes trivially because ${TEMP}/${NSTEPS} contain no V_. Please add a case with ${V_SOMETHING} — it will show the behaviour described in item 3.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dpgen2/exploration/task/lmp_template_task_group.py (1)
129-144: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor strict validation when
revisionsis empty.When
revisions={}, this branch always warns and continues. With the defaultstrict_revisions=True, a template containingV_MISSINGmust fail task generation. The documentation states that strict mode stops generation for any unknown standaloneV_*token.Route
unreplacedthroughreport_undefined_revision_variables(..., strict=self.strict_revisions)and convert its strict-modeValueErrortoFatalError, as in lines 120-128.🤖 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 129 - 144, Update the empty-revisions branch in the task-generation validation to pass unreplaced variables to report_undefined_revision_variables with strict=self.strict_revisions, so strict mode rejects unknown V_* tokens while non-strict mode preserves warnings. Catch the helper’s strict-mode ValueError and convert it to FatalError, matching the existing handling in the nearby validation path.
🤖 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 55-60: Update the set_lmp parameter list so existing positional
arguments retain their original bindings: keep strict_revisions after the
established parameters, or make it keyword-only without changing prior
positional parameters. Ensure calls such as set_lmp(..., revisions, 100)
continue assigning 100 to traj_freq.
- Around line 14-17: Apply the repository’s formatting tools to the dpgen2
package by running ruff format dpgen2/ followed by isort dpgen2/. Ensure the
FatalError import in lmp_template_task_group.py is collapsed and imports are
organized according to the formatter output.
---
Outside diff comments:
In `@dpgen2/exploration/task/lmp_template_task_group.py`:
- Around line 129-144: Update the empty-revisions branch in the task-generation
validation to pass unreplaced variables to report_undefined_revision_variables
with strict=self.strict_revisions, so strict mode rejects unknown V_* tokens
while non-strict mode preserves warnings. Catch the helper’s strict-mode
ValueError and convert it to FatalError, matching the existing handling in the
nearby validation path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3041355d-ac05-42a0-9e0e-251559d9438a
📒 Files selected for processing (4)
docs/input.mddpgen2/exploration/task/lmp_template_task_group.pydpgen2/exploration/task/make_task_group_from_config.pytests/exploration/test_lmp_templ_task_group.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/exploration/test_lmp_templ_task_group.py
| from dflow.python import ( | ||
| FatalError, | ||
| ) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply the required Python formatting.
Run ruff format dpgen2/ and isort dpgen2/ before committing. ruff format collapses this import to one line.
Proposed fix
-from dflow.python import (
- FatalError,
-)
+from dflow.python import FatalErrorAs per coding guidelines, run code formatting and import organization with ruff format dpgen2/ and isort dpgen2/ before committing.
📝 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.
| from dflow.python import ( | |
| FatalError, | |
| ) | |
| from dflow.python import FatalError |
🤖 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 14 - 17,
Apply the repository’s formatting tools to the dpgen2 package by running ruff
format dpgen2/ followed by isort dpgen2/. Ensure the FatalError import in
lmp_template_task_group.py is collapsed and imports are organized according to
the formatter output.
Source: Coding guidelines
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Thanks for the rework — all six points from my previous review are genuinely addressed at 7d458f0c, and I verified that by reconstruction rather than by reading the diff. Two corrections and one blocker.
First, a correction on my side
My previous review was anchored to 44ddb10f, but the head was already 0f17a01a when I submitted. Points 1 (prefix collision) and 4 (comment-stripping asymmetry) were already fixed in 67e57bc7 / 0f17a01a before I posted them. Sorry for the noise — those two threads were never actionable.
Verification of the remaining points
I reverted each fix in isolation and re-ran your tests. They are not vacuous:
- Reverting
revise_by_keysto the unanchoredstr.replaceand disabling check 1 →test_undefined_longer_placeholder_raises_before_substitutionandtest_overlapping_defined_placeholders_are_replaced_as_tokensboth fail, reproducingprint 300 300ERATUREexactly as reported. Note the two fixes are independently sufficient, which is a good belt-and-braces outcome. - Reverting check 3 to
key not in template_raw→test_unused_revision_key_uses_complete_comment_aware_tokensfails. - 17/17 pass at HEAD.
Points 2 (FatalError), 3 (strict_revisions opt-out), 5 (docstring) and 6 (the ${V_MAX} test case) all look right.
Blocker: strict_revisions is not plumbed into customized-lmp-template
This is why build (3.9) and build (3.13) are red. tests.entrypoint.test_submit.TestSubmitCmdStd errors with:
FatalError: LAMMPS template contains undefined revision variable(s):
['V_DIST0', 'V_NSTEPS', 'V_TEMP']. Defined revisions: [].
I reproduced this locally. The escape hatch you added covers lmp-template only:
customized_lmp_template_task_group_args()exposesrevisionsbut nostrict_revisions, so there is no config key a user can set._make_customized_task_group()constructs the innerLmpTemplateTaskGroupand callsset_lmp(...)without passingstrict_revisions, so it defaults toTrue.
So for that task-group type strict validation is unconditional and unescapable. That is not a corner case: in customized-lmp-template the template is generated by the user's own shell command, so carrying V_* tokens with an empty revisions dict is the normal, documented usage. The repo's own example config at test_submit.py#L581 declares no revisions at all while its templates carry V_NSTEPS/V_TEMP/V_DIST0. As it stands this PR makes that configuration unrunnable.
Two ways out, either is fine:
- Add
strict_revisionstocustomized_lmp_template_task_group_args()and thread it through_make_customized_task_group()— but please default it toFalsethere, since an emptyrevisionsis expected for that type and defaulting toTruewould still break every existing config on upgrade. - Skip the raw-template check entirely when
revisionsis empty, on the grounds that a user who defined no revisions is not using the substitution mechanism and has nothing to typo. This also restores the pre-7d458f0cbehaviour thattest_lmp_emptyused to cover.
Either way, TestSubmitCmdStd should go green without being modified — if the fix requires editing that test's config, the compatibility break is still there.
Non-blocking
template_raw is built from self.lmp_template, while the substituted templates come from the plm-revised local lmp_template. No behavioural difference today, since revise_lmp_input_plm only rewrites the PLUMED filename, but the two inputs to the same check reading from different variables is easy to trip over later.
| "strict_revisions", | ||
| bool, | ||
| optional=True, | ||
| default=True, |
There was a problem hiding this comment.
This opt-out exists only on lmp_template_task_group_args(). customized_lmp_template_task_group_args() (further down, ~L247) gets revisions but no strict_revisions, so a customized-lmp-template user has no way to disable the check — and that is exactly the task-group type where an empty revisions dict is normal, because the template is produced by the user's shell command.
That combination is what turns build (3.9) / build (3.13) red on TestSubmitCmdStd. If you add the key here, please default it to False for the customized variant, otherwise existing configs still break on upgrade.
| extra_pair_style_args: str = "", | ||
| pimd_bead: Optional[str] = None, | ||
| input_extra_files: Optional[List[str]] = None, | ||
| strict_revisions: bool = True, |
There was a problem hiding this comment.
CustomizedLmpTemplateTaskGroup._make_customized_task_group() calls set_lmp() without this argument, so it silently takes the True default. Whatever the resolution for the config key, that call site needs to pass the value through explicitly.
| strict=self.strict_revisions, | ||
| ) | ||
| except ValueError as exc: | ||
| raise FatalError(str(exc)) from exc |
There was a problem hiding this comment.
The FatalError wrap addresses my earlier point about exit-code collision with TransientError — thanks. Worth noting for the record that this path is now reached at submit time too (make_lmp_naive_exploration_scheduler), where the exception surfaces to a CLI user rather than to dflow. The message is clear enough that this is fine, just flagging it as intentional.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #366 +/- ##
==========================================
+ Coverage 84.17% 84.37% +0.19%
==========================================
Files 104 104
Lines 6111 6175 +64
==========================================
+ Hits 5144 5210 +66
+ Misses 967 965 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you for the careful review and for clarifying the history of the earlier comments. I implemented option 1 in |
3 participants
Problem
LAMMPS and PLUMED templates may contain DPGEN revision placeholders such as
V_PRESSthat are missing from therevisionsmapping. Without a pre-check,the error is discovered only after remote task submission and queueing.
Solution
Validate revision templates before exploration tasks are created:
V_TEMPandV_TEMP_HIcannot corrupt each other.V_*tokens in the raw and renderedLAMMPS/PLUMED templates with the configured revision keys.
possible unresolved tokens.
FatalErrorso they arenot retried as transient workflow failures.
Compatibility contract
V_*is the established revision-placeholder convention in both dpgen anddpgen2, but native LAMMPS or PLUMED identifiers may legally use the same spelling.
strict_revisionsdefaults totrue: unexpected standaloneV_*tokensstop task generation before submission.
strict_revisionstofalsefor templates that intentionally usenative
V_*identifiers. The tokens are preserved and reported as warnings.${TEMP}, remainuntouched.
Validation coverage
Tests cover LAMMPS and PLUMED templates, empty revisions, unused keys, comments,
quoted hashes, overlapping keys, prefix collisions, native
${V_MAX}identifiers in strict and compatibility modes, and fatal workflow error mapping.
Summary by CodeRabbit