Commit 4c94171
fix: propagate frame-level data modifier failures (#5709)
## Problem
`DeepmdData.get_single_frame()` runs the data modifier through a one-off
`ThreadPoolExecutor` but never calls `future.result()`. The context
manager waits for the submitted task to finish, but any exception raised
inside `modifier.modify_data(...)` stays stored on the `Future` and is
never surfaced. As a result, frame-level data loading silently ignores
modifier failures: callers receive the unmodified frame, and when
caching is enabled that bad frame can be cached for future use. Other
code paths call the modifier directly and do propagate errors, so the
behavior was inconsistent.
## Fix
Call `future.result()` before leaving the modifier block so an exception
raised inside the modifier propagates instead of being swallowed (and
the unmodified frame cached).
## Test
A new test constructs a `DeepmdData` with a modifier whose `modify_data`
always raises, and asserts that `get_single_frame` re-raises the error
and does not cache the failed frame. On the current code the error is
swallowed (no exception, frame cached); after the fix the error
propagates.
Fix #5690
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved error handling when preparing a single frame so failures from
data modifiers are no longer hidden.
* Prevented invalid frames from being saved and reused after a modifier
error.
* **Tests**
* Added coverage to verify modifier failures are reported correctly and
do not populate the frame cache.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Han Wang <wang_han@iapcm.ac.cn>1 parent 9bd1f16 commit 4c94171
2 files changed
Lines changed: 41 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
530 | 530 | | |
531 | 531 | | |
532 | 532 | | |
| 533 | + | |
| 534 | + | |
| 535 | + | |
533 | 536 | | |
534 | 537 | | |
535 | 538 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
46 | 46 | | |
47 | 47 | | |
48 | 48 | | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
0 commit comments