Repository navigation
Conversation
dosubot
Bot
added
size:L
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 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 85.98% 86.03% +0.04%
==========================================
Files 105 105
Lines 6930 6939 +9
==========================================
+ Hits 5959 5970 +11
+ Misses 971 969 -2 ☔ 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.
There was a problem hiding this comment.
Fixed in 4726860 and ea782dc without adding suppressions: normalize HDF5 data to an ndarray, then index the capped diagnostic coordinates explicitly. Actual CI exposed a NumPy 2.2.6 typing difference, so the follow-up was checked with that version too: full-repository pyright 1.1.318 now reports zero errors locally. I deliberately kept validation of all six quantities exported by this loader, including virial metrics, and documented that choice; all six are covered on both input paths. Large finite sentinel values remain accepted.
Agent: dot
| 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.
There was a problem hiding this comment.
Changed this artifact-content failure to dflow FatalError in 4726860, with assertions updated to that exact exception class. NaN and both signs of infinity now exercise FatalError across every exported column on text and HDF5 inputs, so this guard follows dflow's fatal exit path rather than the retryable generic-exception path.
Agent: dot
| 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.
There was a problem hiding this comment.
Fixed in 4726860. HDF5 diagnostics now identify fname.key; the real HDF5 fixture asserts task.000000/model_devi and rejects an object-address message. Diagnostics show at most ten locations plus the total non-finite cell count. A 20,000-frame, 120,000-invalid-cell test checks the cap and a message under 1,000 characters.
Agent: dot
|
|
||
|
|
||
| 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.
There was a problem hiding this comment.
Expanded in 4726860 to test NaN, +inf and -inf in each of the six columns, on both text and real HDF5 datasets. Added finite single/multi-row and DBL_MAX coverage. Both proposed mutations (isnan only and 1:5) now fail. Local expanded regression: 114 tests passed across exploration and SelectConfs; the focused module has 6 tests with 40 subtests. Pinned formatting/import checks and changed-file pyright pass.
Agent: dot
Use typed numeric arrays, report HDF5 dataset keys, and cap diagnostics. Cover NaN and both infinities in all six exported columns on text and HDF5 paths, plus large finite values. Merge current master. Agent: dot
Use explicit two-dimensional indexing instead of scalar/list-sensitive tolist overloads. Full pyright 1.1.318 passes with NumPy 2.2.6; the exploration and SelectConfs regression tests remain green. Agent: dot
Summary
FatalError, preventing retries of invalid artifact contentsValidation
PYTHONPATH=.:tests SKIP_UT_WITH_DFLOW=0 DFLOW_DEBUG=1 python -m pytest -q tests/exploration tests/test_select_confs.py: 114 passedisnanor narrowing columns to1:5fails the suitegit diff --check: passea782dc33abef271c0ce5c83d65fbbe2c101f26bThe validation deliberately covers all six values exported by this loader, including virial metrics, while allowing large finite sentinel values. Report-layer defense in depth is outside this artifact-boundary fix.
Closes #276
Original coding agent: Codex
Review revision: dot
Agent: dot