Preserve model backend suffix in generated tasks - #120
Open
njzjz wants to merge 9 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #120 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 25 25
Lines 6665 6729 +64
=====================================
- Misses 6665 6729 +64 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds dynamic model filename resolution based on model suffixes and task settings. Resolved names propagate through task generation, refinement, runtime symlinking, LAMMPS input generation, and workflow file forwarding across simulation modules. ChangesDynamic model filename resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 3
🤖 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 `@dpti/hti.py`:
- Around line 1510-1515: The _graph_link_command function constructs a shell
command by directly interpolating graph_relpath and model_file into an ln -s
command without proper quoting, which creates a security and stability issue if
these paths contain spaces or shell metacharacters. Fix this by using shell
quoting (such as shlex.quote) to properly escape both the graph_relpath source
argument and the model_file destination argument in the return statement, and
add the -f flag to the ln -s command to make it idempotent. Apply the same fix
to the similar code mentioned in the comment at lines 1547-1550.
In `@dpti/mti.py`:
- Around line 339-343: The model_file variable is being interpolated directly
into shell ln -s commands without proper quoting, creating a security and
robustness issue. In the shell command fragments where link_model is assigned
(around lines 339 and 341, and also in the similar block at lines 374-380), wrap
both instances of the model_file variable with double quotes in the ln -s
commands to properly escape shell-sensitive characters. Additionally, add the -f
flag to the ln -s command to make the linking idempotent, so it will overwrite
existing links without error. Apply these same quoting and -f flag changes to
all occurrences of link_model assignments that use model_file.
In `@dpti/ti.py`:
- Around line 1081-1085: The model_file variable is directly interpolated into
the shell command string without proper quoting, creating a vulnerability to
command injection or path parsing issues if the filename contains special
characters or spaces. In the command string that includes the ln -s symlink
creation, wrap both occurrences of model_file with appropriate shell quotes
(single or double quotes) to properly escape the path. This applies to the
conditional command assignment where model_file is used twice in the symlink
command.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ad7ce01-416a-4024-9b69-d9f051ce24f1
📒 Files selected for processing (18)
dpti/equi.pydpti/gdi.pydpti/hti.pydpti/hti_ice.pydpti/hti_liq.pydpti/hti_water.pydpti/lib/utils.pydpti/mti.pydpti/old_equi.pydpti/relax.pydpti/ti.pytests/test_equi_make_task.pytests/test_gdi_make_task.pytests/test_hti_make_task.pytests/test_lib_utils.pytests/test_ti_make_task.pyworkflow/DpFreeEnergy.pyworkflow/DpFreeEnergyWater.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dpti/ti.py (1)
1081-1084:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShell quoting is still bypassable for crafted filenames.
At Line 1082, wrapping
model_filein double quotes is not enough if the basename contains quotes; this can still break command parsing/injection safety. Please shell-escape both path operands before interpolation.Suggested patch
def run_task(task_name, machine_file): + import shlex @@ - command=( - f'ln -sf "../{model_file}" "{model_file}"; {mdata["command"]} -in in.lammps' - if model_file - else f"{mdata['command']} -in in.lammps" - ), + command=( + f"ln -sf {shlex.quote(f'../{model_file}')} {shlex.quote(model_file)}; {mdata['command']} -in in.lammps" + if model_file + else f"{mdata['command']} -in in.lammps" + ),🤖 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 `@dpti/ti.py` around lines 1081 - 1084, The command construction at the `ln -sf` statement does not properly shell-escape the model_file variable before interpolation, which allows shell injection if the filename contains special characters like quotes. Use a shell escaping function (such as shlex.quote() in Python) to properly escape both the symlink source path (the quoted "../{model_file}" part) and the symlink destination path (the quoted "{model_file}" part) before interpolating them into the f-string command. This ensures that any special characters in the filename cannot break out of the quoted context or inject arbitrary commands.
🤖 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.
Duplicate comments:
In `@dpti/ti.py`:
- Around line 1081-1084: The command construction at the `ln -sf` statement does
not properly shell-escape the model_file variable before interpolation, which
allows shell injection if the filename contains special characters like quotes.
Use a shell escaping function (such as shlex.quote() in Python) to properly
escape both the symlink source path (the quoted "../{model_file}" part) and the
symlink destination path (the quoted "{model_file}" part) before interpolating
them into the f-string command. This ensures that any special characters in the
filename cannot break out of the quoted context or inject arbitrary commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 305627ba-f5f6-43e1-820f-717d3362e822
📒 Files selected for processing (3)
dpti/hti.pydpti/mti.pydpti/ti.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dpti/mti.py
- dpti/hti.py
Member
Author
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Resolve conflicts with template force-field support while preserving dynamic DeepMD model suffixes. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dpti/hti.py (1)
1069-1078: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve MEAM inputs during refinement.
Line 1072 assumes every model-less task has
template_ff. Refining a MEAM HTI task instead raisesKeyError. Branch explicitly on the resolvedif_meamvalue, stage its library/potential into the refined task, and retainmeam_modelbefore calling_make_tasks.🤖 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 `@dpti/hti.py` around lines 1069 - 1078, Update the model-less task handling around the resolved if_meam value instead of assuming template_ff exists. For MEAM tasks, stage the library and potential paths into to_jdata and preserve meam_model before invoking _make_tasks; retain the existing template_ff/template_ff_files handling for non-MEAM tasks.
🤖 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 `@dpti/mti.py`:
- Around line 354-360: Use shlex.quote() for both the relative source path and
destination argument when constructing dynamic model-link shell commands in
dpti/mti.py lines 354-360, covering the link_model branches near job_type
"mass_ti"; apply the same shell-safe quoting to ../{model_file} and {model_file}
in dpti/ti.py line 1127. Ensure both command arguments are quoted after
constructing their paths, rather than relying on double quotes.
---
Outside diff comments:
In `@dpti/hti.py`:
- Around line 1069-1078: Update the model-less task handling around the resolved
if_meam value instead of assuming template_ff exists. For MEAM tasks, stage the
library and potential paths into to_jdata and preserve meam_model before
invoking _make_tasks; retain the existing template_ff/template_ff_files handling
for non-MEAM tasks.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45ffb952-4316-4b1a-bc03-5bd72d16d959
📒 Files selected for processing (11)
dpti/equi.pydpti/gdi.pydpti/hti.pydpti/hti_ice.pydpti/hti_water.pydpti/lib/utils.pydpti/mti.pydpti/ti.pytests/test_gdi_make_task.pytests/test_hti_make_task.pytests/test_lib_utils.py
🚧 Files skipped from review as they are similar to previous changes (5)
- dpti/hti_ice.py
- dpti/gdi.py
- tests/test_lib_utils.py
- dpti/hti_water.py
- tests/test_hti_make_task.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
model.pth->graph.pthTests
PYTHONPATH=.. pytest -q test_lib_utils.py test_hti_make_task.py test_gdi_make_task.py test_equi_make_task.py test_ti_make_task.py test_hti_liq_make_task.py test_hti_liq_gen_lammps_input.py test_hti_water_gen_lammps_input.py test_ti_water_gen_lammps_input.py test_hti_gen_lammps_input.pypython -m ruff check --select I,F --ignore F841 dpti/lib/utils.py dpti/equi.py dpti/gdi.py dpti/hti.py dpti/hti_water.py dpti/hti_liq.py dpti/hti_ice.py dpti/ti.py dpti/mti.py dpti/relax.py dpti/old_equi.py tests/test_lib_utils.py tests/test_hti_make_task.py tests/test_gdi_make_task.py tests/test_ti_make_task.py tests/test_equi_make_task.py workflow/DpFreeEnergy.py workflow/DpFreeEnergyWater.pypython -m compileall -q dpti workflow tests/test_lib_utils.py tests/test_hti_make_task.py tests/test_gdi_make_task.py tests/test_ti_make_task.py tests/test_equi_make_task.pyFull
pytest -qcurrently reports116 passed, 1 error; the remaining error is an existing pytest collection issue intests/test_hti_ff_spring.py:54, where a module-level test function has aselfparameter.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
graph.pb.Tests