Conversation
dosubot
Bot
added
size:L
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Fail at the LAMMPS artifact boundary before NaN or infinity values can produce invalid exploration trust levels. Coding-Agent: Codex Codex-Version: codex-cli 0.149.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
8eb52fb to
22f50a2
Compare
dosubot
Bot
added
size:S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #389 +/- ##
==========================================
+ Coverage 84.43% 84.44% +0.01%
==========================================
Files 104 104
Lines 6110 6115 +5
==========================================
+ Hits 5159 5164 +5
Misses 951 951 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The diagnosis is right and the guard is worth having. A NaN really does reach the report: ExplorationReportAdaptiveLower._record_one_traj buckets with md_f[ii] > level_f_hi, which is False for NaN, so NaN frames fall into the candidate pool, and record then sorts that pool and reads level_f_lo straight out of it. That is the mechanism behind the nan in #276. And I checked the test is not decoration: deleting the whole guard makes it fail.
Three things to fix, then I am happy.
The pyright job is red, on these two lines
traj_render_lammps.py:82:44 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
traj_render_lammps.py:85:81 - error: "intp" is not iterable
Both are new here, and the Type checker workflow is green on master. Every other dd[:, N] access in this function carries # type: ignore for exactly this reason -- dd comes from HDF5Dataset.get_data(), which is optional -- and the new line 82 is the one that does not. Applying the same annotation the surrounding code already uses should clear both.
ValueError will be retried
This runs inside the SelectConfs OP: SelectConfs.execute to ConfSelectorFrames.select to get_model_devi to here. In dflow's generated pod script only TransientError maps to exit 1 and FatalError to exit 2; any other exception propagates and the interpreter also exits 1, which is indistinguishable from transient. The retry expression is literally asInt(lastRetry.exitCode) == 1, and retry_on_transient_error is a documented per-step option that reaches this template through superop/block.py. So for anyone who enables it, a file that can never load will be re-downloaded and re-parsed until the budget runs out.
select_confs.py already raises FatalError five times for this exact class, added in #147 for these very model_devis artifacts. FatalError is the right type here. Note it is not a ValueError subclass, so the new test's assertRaisesRegex needs updating too.
(For the record, get_ele_temp a few lines below does raise a bare ValueError -- but that guards a config integer, not artifact content crossing an OP boundary, so I do not think it is the precedent to follow.)
The message identifies nothing on the HDF5 path
fname is a Path or an HDF5Dataset, and HDF5Dataset defines neither __str__ nor __repr__. With use_hdf5 enabled the message reads
Non-finite model-deviation value in <dflow.python.opio.HDF5Dataset object at 0x7fe5206eb0e0>: row 2 (max_devi_f). ...
while fname.key -- task.000000/model_devi, precisely the thing the message tells the user to go inspect -- is sitting right there unused. The comment above says the point is that "users can inspect the corresponding LAMMPS task directly"; on this branch it does the opposite. The new test only covers the Path branch, which is why this slipped through.
Not blocking
The test proves less than the PR claims. The description says it validates all six columns and rejects NaN and infinity. Two mutations of your own code survive the suite:
~np.isfinite(dd[:, 1:7]) -> np.isnan(dd[:, 1:7]) Ran 3 tests ... OK # infinity no longer rejected
dd[:, 1:7] -> dd[:, 1:5] Ran 3 tests ... OK # two columns dropped
Nothing anywhere in tests/ puts an inf in any column, and no test puts a non-finite value in any column other than max_devi_f. Two more cases would close both.
Scope of the column range. The check covers the three virial columns, and MIN_DEVI_V and AVG_DEVI_V are never read by any report class -- grep the report package, they are written by this loader and consumed nowhere. MAX_DEVI_V is only compared when virial trust levels are configured, and those default to None. So two thirds of the virial half of this check validates data that drives no decision. I want to be careful not to overstate this: I built a fixture with healthy forces and a sentinel virial row and it does newly raise, but that combination is my construction, not something the tracker shows happening -- #276's own attachment has -nan in max_devi_f too, so a check on dd[:, 4] alone would still have closed it. Worth a thought about whether the extra columns earn their risk.
Message size. One clause per non-finite cell, uncapped, and it is largest in exactly the all-NaN case this targets. On #276's pattern I measured 176 KB at 2,000 frames and 1.79 MB at 20,000. Nothing truncates it before the pod log.
The report side is untouched. If this guard is ever narrowed or bypassed, record still sorts NaN into coll_f and still reads level_f_lo from it. Worth a follow-up issue rather than scope creep here.
One thing I checked and want to record as a non-issue: 1.797693e+308 in the #276 file is finite and passes the guard, but that is harmless. DBL_MAX > level_f_hi is True, so those frames are correctly counted as failed and level_f_lo falls back to the configured value. It is only NaN that slips through the comparison.
| DeviManager.MIN_DEVI_F, | ||
| DeviManager.AVG_DEVI_F, | ||
| ) | ||
| invalid = np.argwhere(~np.isfinite(dd[:, 1:7])) |
There was a problem hiding this comment.
This line is one of the two that turn the pyright job red:
traj_render_lammps.py:82:44 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
dd can be None because it comes from HDF5Dataset.get_data(), which is why the reshape above and all six model_devi.add(dd[:, N]) calls below carry # type: ignore. This new access is the only one without it. Line 85's for row, column in invalid draws the second error ("intp" is not iterable). The Type checker workflow is green on master, so both are new here.
Separately on scope: this slice covers the three virial columns, and MIN_DEVI_V and AVG_DEVI_V are read by no report class at all, while MAX_DEVI_V is only compared when virial trust levels are set, which they are not by default. I am not asking you to change it, but it is worth deciding deliberately: dd[:, 4] alone would still have closed #276, since that file's max_devi_f is -nan too.
| locations = ", ".join( | ||
| f"row {row + 1} ({deviation_names[column]})" for row, column in invalid | ||
| ) | ||
| raise ValueError( |
There was a problem hiding this comment.
This should be FatalError, not ValueError.
_load_one_model_devi runs inside the SelectConfs OP (SelectConfs.execute to ConfSelectorFrames.select to get_model_devi to here). dflow's generated pod script catches only TransientError (exit 1) and FatalError (exit 2); anything else propagates and the interpreter also exits 1, which is indistinguishable from transient. The retry expression is asInt(lastRetry.exitCode) == 1, and retry_on_transient_error is a documented step option wired to this template via superop/block.py. So a file that can never parse gets re-downloaded and re-parsed until the retry budget is gone -- the opposite of the fail-fast this guard is for.
select_confs.py raises FatalError five times for this same class of artifact-boundary problem, added in #147 for these very model_devis inputs. Note FatalError is not a ValueError subclass, so the new test's assertRaisesRegex needs to change with it.
| f"row {row + 1} ({deviation_names[column]})" for row, column in invalid | ||
| ) | ||
| raise ValueError( | ||
| f"Non-finite model-deviation value in {fname}: {locations}. " |
There was a problem hiding this comment.
fname is a Path or an HDF5Dataset, and HDF5Dataset defines neither __str__ nor __repr__. With use_hdf5 enabled this renders as:
Non-finite model-deviation value in <dflow.python.opio.HDF5Dataset object at 0x7fe5206eb0e0>: row 2 (max_devi_f). ...
fname.key holds task.000000/model_devi -- exactly the identifier the rest of the sentence tells the user to go inspect -- and it is discarded. Something like fname.key if isinstance(fname, HDF5Dataset) else fname restores the intent. The new test only exercises the Path branch, so this is not caught.
While you are here: locations emits one clause per non-finite cell with no cap, and the all-NaN case this targets is the worst one. On #276's pattern I measured 176 KB at 2,000 frames and 1.79 MB at 20,000, all on one line into the pod log. Capping at the first few plus a total count would read better.
|
|
||
|
|
||
| class TestTrajRenderLammps(unittest.TestCase): | ||
| def test_rejects_non_finite_model_deviation(self): |
There was a problem hiding this comment.
Not blocking, but this pins less than the PR claims. The description says all six columns and NaN and infinity; this covers one NaN in one column. Both of these mutations of the source survive the suite:
~np.isfinite(dd[:, 1:7]) -> np.isnan(dd[:, 1:7]) Ran 3 tests ... OK
dd[:, 1:7] -> dd[:, 1:5] Ran 3 tests ... OK
I checked the rest of tests/ too: no test anywhere puts an inf in any column, and none puts a non-finite value in any column other than max_devi_f. An inf case and one non-force column would close both gaps.
Credit where due -- the test is not vacuous. Removing the guard entirely does make it fail.
2 participants
Summary
Tests
PYTHONPATH=tests python -m unittest -v tests.exploration.test_traj_render_lammpsruff format --check dpgen2/exploration/render/traj_render_lammps.py tests/exploration/test_traj_render_lammps.pyisort --check-only dpgen2/exploration/render/traj_render_lammps.py tests/exploration/test_traj_render_lammps.pygit diff --checkCloses #276
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh