Skip to content

feat: validate LAMMPS template revision variables before task execution - #366

Open
SchrodingersCattt wants to merge 17 commits into
deepmodeling:masterfrom
SchrodingersCattt:feat/lmp-variable-precheck
Open

feat: validate LAMMPS template revision variables before task execution#366
SchrodingersCattt wants to merge 17 commits into
deepmodeling:masterfrom
SchrodingersCattt:feat/lmp-variable-precheck

Conversation

@SchrodingersCattt

@SchrodingersCattt SchrodingersCattt commented Jul 20, 2026

Copy link
Copy Markdown

Problem

LAMMPS and PLUMED templates may contain DPGEN revision placeholders such as
V_PRESS that are missing from the revisions mapping. Without a pre-check,
the error is discovered only after remote task submission and queueing.

Solution

Validate revision templates before exploration tasks are created:

  1. Replace revision keys as complete tokens so overlapping names such as
    V_TEMP and V_TEMP_HI cannot corrupt each other.
  2. In strict mode, compare standalone V_* tokens in the raw and rendered
    LAMMPS/PLUMED templates with the configured revision keys.
  3. Warn when a configured revision key is unused.
  4. When no revisions are configured, preserve existing behavior and warn about
    possible unresolved tokens.
  5. Raise deterministic validation failures as dflow FatalError so they are
    not retried as transient workflow failures.

Compatibility contract

V_* is the established revision-placeholder convention in both dpgen and
dpgen2, but native LAMMPS or PLUMED identifiers may legally use the same spelling.

  • strict_revisions defaults to true: unexpected standalone V_* tokens
    stop task generation before submission.
  • Set strict_revisions to false for templates that intentionally use
    native V_* identifiers. The tokens are preserved and reported as warnings.
  • LAMMPS references without the reserved prefix, such as ${TEMP}, remain
    untouched.

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

  • New Features
    • Added configurable strict validation for revision tokens in LAMMPS and PLUMED templates.
    • Strict mode is enabled by default; warning-only behavior can be selected when undefined tokens should remain.
  • Bug Fixes
    • Improved detection of unresolved and unused revision variables.
    • Revision replacement now matches complete tokens accurately.
    • Validation correctly handles comments, quoted content, internal variables, and overlapping placeholders.
  • Documentation
    • Documented the new strict validation option and configuration setting.

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 dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Jul 20, 2026
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 20, 2026
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@SchrodingersCattt, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 seconds

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bf89ff6f-2f74-4806-9cf9-240e36e67c99

📥 Commits

Reviewing files that changed from the base of the PR and between ca29071 and 7d458f0.

📒 Files selected for processing (2)
  • dpgen2/exploration/task/lmp_template_task_group.py
  • tests/exploration/test_lmp_templ_task_group.py
📝 Walkthrough

Walkthrough

LAMMPS and PLUMED template task creation now validates unresolved V_* revision placeholders. Strict mode raises FatalError; non-strict mode emits warnings. Configuration and documentation expose the setting. Tests cover token matching, comments, quoting, unused keys, and PLUMED templates.

Changes

Revision Placeholder Validation

Layer / File(s) Summary
Placeholder detection and substitution
dpgen2/exploration/task/lmp_template_task_group.py
Replacement now matches complete revision tokens. Validation ignores unquoted LAMMPS comments and detects remaining standalone V_* placeholders.
Task creation validation and configuration
dpgen2/exploration/task/lmp_template_task_group.py, dpgen2/exploration/task/make_task_group_from_config.py, docs/input.md
set_lmp and lmp-template configuration accept strict_revisions, which defaults to True. Task creation validates raw and rendered templates, raises FatalError for undefined variables in strict mode, and warns in non-strict mode.
Revision validation coverage
tests/exploration/test_lmp_templ_task_group.py
Tests cover undefined, unused, empty, overlapping, quoted, commented, PLUMED, and internal LAMMPS variables in strict and warning modes.

Estimated code review effort: 4 (Complex) | ~45 minutes

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: pre-execution validation of revision variables in LAMMPS templates.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
dpgen2/exploration/task/lmp_template_task_group.py (1)

314-320: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Consider using word boundaries for unused key detection.

Checking if key not in template_raw uses simple substring matching. If revisions defines V_NSTEPS, but the template only uses a longer variable like V_NSTEPS_1, the substring match will still evaluate to True, inadvertently suppressing the unused key warning for V_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

📥 Commits

Reviewing files that changed from the base of the PR and between b05af11 and 87c626a.

📒 Files selected for processing (2)
  • dpgen2/exploration/task/lmp_template_task_group.py
  • tests/exploration/test_lmp_templ_task_group.py

Comment thread dpgen2/exploration/task/lmp_template_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.
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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("#")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 njzjz-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Use complete placeholder matching for unused revision keys.

Line 378 uses a substring check. A template that contains only V_TEMPERATURE makes configured V_TEMP appear used. A V_TEMP occurrence in a comment also suppresses the warning.

Use raw_variables, which already applies the required token and comment rules. Add a regression for configured V_TEMP with a raw template that contains only V_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

📥 Commits

Reviewing files that changed from the base of the PR and between 87c626a and bb912df.

📒 Files selected for processing (2)
  • dpgen2/exploration/task/lmp_template_task_group.py
  • tests/exploration/test_lmp_templ_task_group.py

@wanghan-iapcm wanghan-iapcm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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              # -> warning

fixes 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:

doc_plm_template_fname = "The file name of plumed input template"
doc_revisions = "The revisions. Should be a dict providing the key - list of desired values pair. Key is the word to be replaced in the templates, and it may appear in both the lammps and plumed input templates. All values in the value list will be enmerated."
doc_traj_freq = "The frequency of dumping configurations and thermodynamic states"

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))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-148 only catches FatalError and re-wraps it with stage context. A ValueError escapes that, so the operator loses which stage failed.
  • dflow's generated OP script special-cases TransientError -> exit 1 and FatalError -> exit 2. An uncaught ValueError falls through to Python's default uncaught-exception code, which is also 1 — indistinguishable from TransientError. A retry_on_transient_error strategy 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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 warning

Commenting 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Honor strict validation when revisions is empty.

When revisions={}, this branch always warns and continues. With the default strict_revisions=True, a template containing V_MISSING must fail task generation. The documentation states that strict mode stops generation for any unknown standalone V_* token.

Route unreplaced through report_undefined_revision_variables(..., strict=self.strict_revisions) and convert its strict-mode ValueError to FatalError, 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

📥 Commits

Reviewing files that changed from the base of the PR and between bb912df and ca29071.

📒 Files selected for processing (4)
  • docs/input.md
  • dpgen2/exploration/task/lmp_template_task_group.py
  • dpgen2/exploration/task/make_task_group_from_config.py
  • tests/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

Comment on lines +14 to 17
from dflow.python import (
FatalError,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 FatalError

As 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.

Suggested change
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

Comment thread dpgen2/exploration/task/lmp_template_task_group.py Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants