Fix NMD crash on un-mappable reactions and OS memory over-allocation trsh#909
Open
alongd wants to merge 2 commits into
Open
Fix NMD crash on un-mappable reactions and OS memory over-allocation trsh#909alongd wants to merge 2 commits into
alongd wants to merge 2 commits into
Conversation
analyze_ts_normal_mode_displacement() raised a ReactionError when reaction.atom_map is None (e.g. reactions ARC cannot atom-map, such as CH3 + CS <=> CCS with a charged CS), aborting the whole run. Guard the check to log a warning and return None when there is no atom map, so the scheduler continues and simply skips NMD validation for that reaction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #909 +/- ##
==========================================
- Coverage 63.16% 63.09% -0.07%
==========================================
Files 114 114
Lines 38177 38189 +12
Branches 9990 9992 +2
==========================================
- Hits 24114 24095 -19
- Misses 11181 11205 +24
- Partials 2882 2889 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers two targeted robustness fixes in ARC: (1) preventing TS normal-mode-displacement (NMD) validation from aborting runs when a reaction cannot be atom-mapped, and (2) distinguishing OS/node memory overallocation failures from “Gaussian needs more memory”, enabling appropriate (decreasing) memory troubleshooting.
Changes:
- Skip NMD validation (with a warning) when
reaction.atom_mapis unavailable, instead of raising and terminating the run. - Classify Gaussian
galloc/malloc failedasMemoryOverallocationand troubleshoot by reducing requested memory down to a 4 GB floor. - Add/extend unit tests and a Gaussian output fixture to cover both behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| arc/checks/nmd.py | Adds an early guard to skip NMD analysis when no atom map is available. |
| arc/checks/nmd_test.py | Adds a regression test ensuring unmappable reactions return None instead of crashing. |
| arc/job/trsh.py | Introduces MemoryOverallocation classification and implements “reduce memory” troubleshooting for OS allocation failures. |
| arc/job/trsh_test.py | Adds tests for status classification and the reduce-then-give-up troubleshooting path. |
| arc/testing/trsh/gaussian/galloc.out | Adds a Gaussian fixture exhibiting galloc: could not allocate memory. for regression testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+983
to
+991
| if 'MemoryOverallocation' in job_status['keywords'] and server is not None: | ||
| reduced_memory = max(memory_gb / 2.0, 4) | ||
| if reduced_memory < memory_gb: | ||
| couldnt_trsh = False | ||
| memory = reduced_memory | ||
| logger.info(f'Troubleshooting {job_type} job in {software} for {label} using less memory: {memory} GB ' | ||
| f'instead of {memory_gb} GB (the node could not allocate the requested amount)') | ||
| ess_trsh_methods.append('memory') | ||
| else: |
Comment on lines
+993
to
+996
| output_errors.append( | ||
| f'Error: Could not troubleshoot {job_type} for {label}! The node could not allocate even ' | ||
| f'{memory_gb} GB, and ARC is already at its minimum memory request; use a higher-memory node. ') | ||
| logger.error(f'Could not troubleshoot {job_type} job in {software} for {label}. The node could not ' |
A Gaussian galloc/malloc failure means the node could not provide the requested memory (we asked for too much) - the opposite of Gaussian internally requesting more memory. Previously both were classified as 'Memory' and troubleshooting increased the request, making a node-capacity failure worse. Classify the OS-level failure as 'MemoryOverallocation' and troubleshoot it by halving the request down to a 4 GB floor, giving up with a clear 'use a higher-memory node' message once at the minimum. Adds the galloc.out fixture and tests for both the status classification and the reduce-then-give-up troubleshooting path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Two independent, self-contained bug fixes surfaced and validated during an end-to-end run (
linearTS-adapter validation, wB97XD/def2-TZVP, Gaussian). Each was confirmed on real jobs during the run, and each ships with unit tests.1. NMD check crashes on un-atom-mappable reactions
analyze_ts_normal_mode_displacement()raised aReactionError(Cannot get bonds ...) whenreaction.atom_map is None, aborting the entire ARC run. This happens for reactions ARC cannot atom-map — e.g.CH3 + CS <=> CCS(R_Addition_MultipleBondwith a charged[C-]#[S+]).Fix: guard the check to log a warning and return
Nonewhen there is no atom map, so the scheduler skips NMD validation for that one reaction and continues.arc/checks/nmd.py, test inarc/checks/nmd_test.py.2. Conflating OS memory over-allocation with Gaussian needing more memory
A Gaussian
galloc/malloc failederror means the node could not provide the requested memory (we asked for too much) — the opposite of Gaussian internally requesting more memory. Both were classified asMemory, so troubleshooting increased the request, making a node-capacity failure strictly worse.Fix: classify the OS-level failure as
MemoryOverallocationand troubleshoot it by halving the request down to a 4 GB floor, giving up with a clear "use a higher-memory node" message once at the minimum.arc/job/trsh.py, tests inarc/job/trsh_test.py, fixturearc/testing/trsh/gaussian/galloc.out.Validation
🤖 Generated with Claude Code