Fix global mapping annotation contamination in parsers#137
Fix global mapping annotation contamination in parsers#137ndaelman-hu wants to merge 27 commits into
Conversation
…W/NC-PAW/NC-PAW-GW
Previously the load() method would catch exceptions, log them, but return None implicitly. This caused assertion failures in NOMAD's parser loading infrastructure which expects a Parser instance. Now exceptions are re-raised after logging to allow proper error handling upstream.
Add `cutoff_target` field to track code-native cutoff terminology (defaults to 'ENMAX' for VASP). Add `sha256` field to store POTCAR hash for unique pseudopotential identification. Update parser to extract SHA256 from OUTCAR and explicitly set cutoff_target when storing cutoff values. Add detailed TODO documenting VASP's norm-conserving pseudopotential detection logic using LPAW and LULTRA flags.
Replace manual field assignment with mapping annotation system. Store raw POTCAR data in m_cache with OUTCAR_KEY to enable automatic field mapping and unit conversion defined in vasp.py schema annotations. Manual assignment now only handles fields without mapping annotations (cutoff, cutoff_target, type, norm_conserving, gw_optimized). Use ureg.eV for cutoff unit conversion instead of manual constant.
Correct pseudopotential field name to match schema definition. The schema defines but parser was setting the non-existent field.
Mapping annotations don't work for pseudopotentials since they're processed after the convert() step. Directly assign all fields from pp_data: - name, n_valence_electrons, reference_configuration, r_core - enmax, enmin (VASP-specific) - sha256 (POTCAR hash) All fields now properly appear in parsed output.
Schema changes (`vasp.py`): - Add `Pseudopotential.m_def` mapping with transformer to filter empty POTCAR entries - Add mappings for VASP-specific fields (`enmax`, `enmin`, `sha256`) - Position mapping after class definition to instantiate VASP extension Parser changes (`outcar_parser.py`): - Add `get_pseudopotentials()` transformer to filter empty dicts - Refactor `_process_pseudopotentials()` to post-process mapped instances - Remove manual assignments for simple fields handled by mapping annotations - Retain post-processing for complex logic (type, XC functional, linking) Uses declarative mappings for field extraction, imperative code for business logic.
Changes: - Add `lpaw`, `lultra`, `lexch` fields to VASP `Pseudopotential` schema - Add mapping annotations for these fields to populate from OUTCAR - Update `_process_pseudopotentials()` to read from instance properties instead of m_cache - Remove TODO about double-checking norm-conserving logic The type determination now correctly identifies: - PAW potentials (LPAW=T, not norm-conserving) - NC-PAW-GW potentials (LPAW=T + _GW suffix, norm-conserving) - US potentials (LULTRA=T, not norm-conserving) - NC potentials (LPAW=F and LULTRA=F, norm-conserving) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add get_pseudopotentials() transformer to extract name and n_valence_electrons from vasprun.xml atomtypes array. Add TODO for direct POTCAR parsing to enable complete pseudopotential metadata (LPAW, LULTRA, LEXCH, cutoffs, etc.) when only vasprun.xml is available.
Extract basic pseudopotential data (name, n_valence_electrons) from vasprun.xml atomtypes array. Created `_add_pseudopotentials()` method in XMLArchiveWriter that navigates modeling.atominfo structure and adds pseudopotentials to model_method.numerical_settings. Also fixed model_method creation from vasprun.xml by simplifying DFT mapping path from `.parameters.separator[?"@name"=='electronic']` to `.parameters`. Note: vasprun.xml provides only basic pseudopotential info while OUTCAR contains complete POTCAR metadata. Direct POTCAR parsing would enable full support regardless of mainfile.
These fields are internal POTCAR flags used only during parsing to derive `type` and `is_norm_conserving`. They should not be stored in the final schema. Changes: - Removed lpaw, lultra, lexch as Quantity fields from vasp.Pseudopotential - Updated `_process_pseudopotentials()` to access these flags from raw parser data instead of schema instance - Pass parser data to `_process_pseudopotentials()` to access intermediate extraction values - Filter empty pseudopotential dicts (OUTCAR extracts 4 entries, first 2 are empty headers) The derived fields (type, is_norm_conserving, cutoff_target, xc_functional) are correctly populated while lpaw/lultra/lexch remain internal to the parser.
Extract maximum angular momentum (LMAX) and number of lm-projection operators (LMMAX) from OUTCAR POTCAR sections. These values describe the completeness of the PAW projection operators. Changes: - Add lmax and lmmax extraction in outcar_parser.py function - Add lmax and lmmax Quantity fields to vasp.Pseudopotential schema - Add mapping annotations for OUTCAR extraction Example values: Mn_pv has LMAX=6, LMMAX=18; S has LMAX=4, LMMAX=8
The reload(vasp) call was breaking metainfo registration by creating new class instances with different m_def objects. Mapping annotations added to the reloaded m_def were not recognized by NOMAD's plugin system, which still referenced the original m_def from initial import. Changes: - Remove reload(vasp) from parser.py that created class instance conflicts - Remove remove_mapping_annotations() since annotations now persist correctly - Add comments explaining why vasp import must remain despite appearing unused - Enhance test_outcar with assertions to verify numerical_settings population Result: OUTCAR-only entries now correctly show numerical_settings with Pseudopotential objects containing complete metadata (name, n_valence_electrons, lmax, sha256).
- Update test to reference `l_max` schema property instead of `lmax` - Fix missing `model_method` mapping annotation for OUTCAR parser
The explicit mapping for `general.Simulation.model_method` was interfering with automatic population by `DFT.m_def`. The mapping is created implicitly and should not be explicitly defined.
Change `DFT.m_def` path from `'parameters'` to `'.parameters'` to be consistent with other OUTCAR mappings that use dot-prefixed paths relative to root (`@`).
Remove broken `reload()` and `remove_mapping_annotations()` pattern that caused parser failures when running in the same process. ## Problem Mapping annotations are stored on class-level `m_def` objects shared globally across all parser instances. Two antipatterns caused contamination: 1. `reload(schema_module)` creates new class instances, but NOMAD's plugin system still references original `m_def`, breaking annotation lookup 2. `remove_mapping_annotations()` removes annotations from shared global state, breaking subsequent parses This caused VASP test failures in full suite and potential production issues in sequential parsing. ## Solution - Remove `reload()` calls from 10 parsers - Remove `remove_mapping_annotations()` calls from 3 parsers - Delete `remove_mapping_annotations()` utility function - Revert VASP subprocess isolation workaround Annotations now correctly persist as static metadata for process lifetime. ## Testing All 42 parser tests pass in both isolation and full suite.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical global state contamination issue where mapping annotations were being dynamically removed after parsing, causing failures when multiple parsers ran in the same process. The fix removes the problematic reload() and remove_mapping_annotations() calls, treating annotations as static metadata that persists for the process lifetime.
Key changes:
- Removed
reload()calls from 11 parsers (VASP, Wannier90, QuantumEspresso, Octopus, H5MD, GPAW, FHI-aims, Exciting, Crystal, AMS, Abinit) - Deleted the
remove_mapping_annotations()utility function - Reverted VASP subprocess isolation workaround to standard tests
- Added comprehensive VASP pseudopotential support with proper OUTCAR parsing
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/nomad_simulation_parsers/schema_packages/utils.py |
Removed remove_mapping_annotations() function that was contaminating global state |
src/nomad_simulation_parsers/schema_packages/vasp.py |
Added Pseudopotential schema class with mapping annotations; modified DFT mapping paths |
src/nomad_simulation_parsers/parsers/__init__.py |
Modified error handling to re-raise exceptions; updated VASP regex patterns |
src/nomad_simulation_parsers/parsers/vasp/parser.py |
Removed reload() and remove_mapping_annotations() calls with explanatory comments |
src/nomad_simulation_parsers/parsers/vasp/xml_parser.py |
Added pseudopotential extraction from vasprun.xml atomtypes |
src/nomad_simulation_parsers/parsers/vasp/outcar_parser.py |
Added comprehensive POTCAR header parsing and pseudopotential post-processing |
src/nomad_simulation_parsers/parsers/wannier90/parser.py |
Removed reload() call |
src/nomad_simulation_parsers/parsers/quantumespresso/parser.py |
Removed reload() call |
src/nomad_simulation_parsers/parsers/octopus/parser.py |
Removed reload() call |
src/nomad_simulation_parsers/parsers/h5md/parser.py |
Removed reload() and remove_mapping_annotations() calls |
src/nomad_simulation_parsers/parsers/gpaw/parser.py |
Removed reload() call |
src/nomad_simulation_parsers/parsers/fhiaims/parser.py |
Removed reload() and remove_mapping_annotations() calls |
src/nomad_simulation_parsers/parsers/exciting/parser.py |
Removed reload() and remove_mapping_annotations() calls |
src/nomad_simulation_parsers/parsers/crystal/parser.py |
Removed reload() call |
src/nomad_simulation_parsers/parsers/ams/parser.py |
Removed reload() call |
src/nomad_simulation_parsers/parsers/abinit/parser.py |
Removed reload() call |
tests/parsers/test_vasp_parser.py |
Reverted subprocess workaround; added assertions for pseudopotential validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sha256_match = re.search(r'SHA256\s*=\s*([a-f0-9]{64})', val_in) | ||
| if sha256_match: | ||
| data['sha256'] = sha256_match.group(1) |
There was a problem hiding this comment.
The SHA256 regex pattern only matches lowercase hexadecimal characters [a-f0-9]. However, SHA256 hashes can be represented in uppercase as well. Consider using a case-insensitive pattern [a-fA-F0-9] or (?i)[a-f0-9] to handle both cases, or convert the matched value to lowercase before storing.
| sha256_match = re.search(r'SHA256\s*=\s*([a-f0-9]{64})', val_in) | |
| if sha256_match: | |
| data['sha256'] = sha256_match.group(1) | |
| sha256_match = re.search(r'SHA256\s*=\s*([A-Fa-f0-9]{64})', val_in) | |
| if sha256_match: | |
| data['sha256'] = sha256_match.group(1).lower() |
| # Link each AtomsState to its corresponding Pseudopotential | ||
| for atoms_state, pp in zip( | ||
| model_system.particle_states, pseudopotentials | ||
| ): |
There was a problem hiding this comment.
The zip function silently truncates to the shorter of the two lists. If there's a mismatch between the number of particle_states and pseudopotentials, some atoms won't have pseudopotentials assigned (if fewer pseudopotentials) or some pseudopotentials won't be used (if more pseudopotentials). This could happen if the OUTCAR format changes or is incomplete. Consider validating that the lengths match and logging a warning or raising an error if they don't, to make debugging easier when this assumption is violated.
| # Link each AtomsState to its corresponding Pseudopotential | |
| for atoms_state, pp in zip( | |
| model_system.particle_states, pseudopotentials | |
| ): | |
| particle_states = model_system.particle_states | |
| num_particle_states = len(particle_states) | |
| num_pseudopotentials = len(pseudopotentials) | |
| if num_particle_states != num_pseudopotentials: | |
| LOGGER.warning( | |
| 'Mismatch between number of particle_states (%d) and pseudopotentials (%d); ' | |
| 'linking will proceed using the minimum of the two.', | |
| num_particle_states, | |
| num_pseudopotentials, | |
| ) | |
| # Link each AtomsState to its corresponding Pseudopotential | |
| for atoms_state, pp in zip(particle_states, pseudopotentials): |
|
|
||
| # Check for Pseudopotentials | ||
| pseudopotentials = [ | ||
| ns for ns in method.numerical_settings if type(ns).__name__ == 'Pseudopotential' |
There was a problem hiding this comment.
Using string comparison type(ns).__name__ == 'Pseudopotential' is fragile and non-Pythonic. If the class is moved to a different module or renamed, this test will fail silently. Use isinstance(ns, vasp.Pseudopotential) instead for a more robust type check. This would require importing the vasp module, which would be a better practice anyway.
| ns for ns in method.numerical_settings if type(ns).__name__ == 'Pseudopotential' | |
| ns | |
| for ns in method.numerical_settings | |
| if hasattr(ns, 'name') and hasattr(ns, 'n_valence_electrons') |
| r'^\s*<\?xml version="1\.0" encoding="ISO-8859-1"\?>\s*?\s*<modeling>?\s*' | ||
| r'<generator>?\s*<i name="program" type="string">\s*vasp\s*</i>?|' | ||
| r'^\svasp[\.\d]+.+?(?:\(build|complex)[\s\S]+?executed on' | ||
| r'^\s*vasp[\.\d]+.+?(?:\(build|complex)[\s\S]+?executed on' |
There was a problem hiding this comment.
The regex pattern has a potential issue: it starts with ^\s*vasp which means it requires the start of string/line, but the backslash-s (\s*) allows leading whitespace. However, the original pattern was ^\svasp (requiring exactly one whitespace character). The new pattern ^\s* (zero or more whitespace) is more permissive. While this change might be intentional to make the parser more flexible, it's unrelated to the PR's stated purpose of fixing global state contamination. If this change is intentional, it should be documented or moved to a separate PR.
| '.parameters', | ||
| ) | ||
| add_mapping_annotation(model_method.DFT.m_def, OUTCAR_KEY, 'parameters') | ||
| add_mapping_annotation(model_method.DFT.m_def, OUTCAR_KEY, '.parameters') |
There was a problem hiding this comment.
The OUTCAR mapping path changed from parameters to .parameters (added leading dot). While both might work depending on the parser's path resolution, this inconsistency with the original code could indicate either a bug fix or an introduction of a bug. The change should be validated to ensure it correctly resolves the path in the OUTCAR data structure.
| add_mapping_annotation(model_method.DFT.m_def, OUTCAR_KEY, '.parameters') | |
| add_mapping_annotation(model_method.DFT.m_def, OUTCAR_KEY, 'parameters') |
| def get_pseudopotentials( | ||
| self, atomtypes: list[dict[str, Any]] | ||
| ) -> list[dict[str, Any]]: | ||
| """ | ||
| Extract basic pseudopotential data from vasprun.xml atomtypes array. | ||
|
|
||
| The atomtypes array contains: | ||
| - atomspertype (count) | ||
| - element | ||
| - mass | ||
| - valence (n_valence_electrons) | ||
| - pseudopotential (name/TITEL) | ||
|
|
||
| Note: vasprun.xml does NOT contain detailed POTCAR metadata (LPAW, LULTRA, | ||
| LEXCH, ENMAX, ENMIN, RCORE, VRHFIN, SHA256). These are only available in | ||
| OUTCAR or by parsing POTCAR directly. | ||
| """ | ||
| LOGGER.info(f'get_pseudopotentials called with type: {type(atomtypes)}') | ||
|
|
||
| if not atomtypes: | ||
| LOGGER.debug('get_pseudopotentials: No atomtypes provided') | ||
| return [] | ||
|
|
||
| LOGGER.debug(f'get_pseudopotentials: Processing {len(atomtypes)} atomtypes') | ||
|
|
||
| pseudopotentials = [] | ||
| for i, atomtype in enumerate(atomtypes): | ||
| LOGGER.debug(f'Item {i}: type={type(atomtype)}') | ||
| if isinstance(atomtype, dict): | ||
| LOGGER.debug(f' keys: {list(atomtype.keys())}') | ||
|
|
||
| # Extract fields from the rc array structure | ||
| # Format: [atomspertype, element, mass, valence, pseudopotential_name] | ||
| rc = atomtype.get('rc', []) | ||
| if not rc or len(rc) < ATOMTYPE_RC_EXPECTED_LENGTH: | ||
| LOGGER.debug(f'get_pseudopotentials: Skipping atomtype with rc={rc}') | ||
| continue | ||
|
|
||
| pp_data = { | ||
| 'name': rc[4].strip(), # pseudopotential name (TITEL) | ||
| 'n_valence_electrons': float(rc[3]), # valence | ||
| } | ||
| pseudopotentials.append(pp_data) | ||
| LOGGER.debug(f'get_pseudopotentials: Added {pp_data["name"]}') | ||
|
|
||
| LOGGER.debug( | ||
| f'get_pseudopotentials: Returning {len(pseudopotentials)} pseudopotentials' | ||
| ) | ||
| return pseudopotentials | ||
|
|
||
|
|
There was a problem hiding this comment.
The get_pseudopotentials method is defined in the VasprunParser class but returns data that is never actually used by the mapping annotations. The JMESPath in the annotation (line 200 in vasp.py) tries to extract atomtypes but this transformer method would process them. However, the _add_pseudopotentials method (lines 128-201) manually extracts and processes the atominfo data instead of relying on the mapping annotation. This creates redundant code paths and confusion about which method is actually used. Consider either using the mapping annotation with this transformer or removing it if the manual extraction in _add_pseudopotentials is the intended approach.
| def get_pseudopotentials( | |
| self, atomtypes: list[dict[str, Any]] | |
| ) -> list[dict[str, Any]]: | |
| """ | |
| Extract basic pseudopotential data from vasprun.xml atomtypes array. | |
| The atomtypes array contains: | |
| - atomspertype (count) | |
| - element | |
| - mass | |
| - valence (n_valence_electrons) | |
| - pseudopotential (name/TITEL) | |
| Note: vasprun.xml does NOT contain detailed POTCAR metadata (LPAW, LULTRA, | |
| LEXCH, ENMAX, ENMIN, RCORE, VRHFIN, SHA256). These are only available in | |
| OUTCAR or by parsing POTCAR directly. | |
| """ | |
| LOGGER.info(f'get_pseudopotentials called with type: {type(atomtypes)}') | |
| if not atomtypes: | |
| LOGGER.debug('get_pseudopotentials: No atomtypes provided') | |
| return [] | |
| LOGGER.debug(f'get_pseudopotentials: Processing {len(atomtypes)} atomtypes') | |
| pseudopotentials = [] | |
| for i, atomtype in enumerate(atomtypes): | |
| LOGGER.debug(f'Item {i}: type={type(atomtype)}') | |
| if isinstance(atomtype, dict): | |
| LOGGER.debug(f' keys: {list(atomtype.keys())}') | |
| # Extract fields from the rc array structure | |
| # Format: [atomspertype, element, mass, valence, pseudopotential_name] | |
| rc = atomtype.get('rc', []) | |
| if not rc or len(rc) < ATOMTYPE_RC_EXPECTED_LENGTH: | |
| LOGGER.debug(f'get_pseudopotentials: Skipping atomtype with rc={rc}') | |
| continue | |
| pp_data = { | |
| 'name': rc[4].strip(), # pseudopotential name (TITEL) | |
| 'n_valence_electrons': float(rc[3]), # valence | |
| } | |
| pseudopotentials.append(pp_data) | |
| LOGGER.debug(f'get_pseudopotentials: Added {pp_data["name"]}') | |
| LOGGER.debug( | |
| f'get_pseudopotentials: Returning {len(pseudopotentials)} pseudopotentials' | |
| ) | |
| return pseudopotentials |
| # NOTE: Pseudopotential annotations registered after class definition (line 203) | ||
| # Ensures proper parser hierarchy: Simulation -> ModelMethod -> NumericalSettings |
There was a problem hiding this comment.
The comment references "line 203" but that's not accurate in the actual code. The pseudopotential annotations are registered at lines 187-201, not at line 203. This will confuse future maintainers trying to understand the code structure.
| # NOTE: Pseudopotential annotations registered after class definition (line 203) | |
| # Ensures proper parser hierarchy: Simulation -> ModelMethod -> NumericalSettings | |
| # NOTE: Pseudopotential annotations are registered after this class definition | |
| # to ensure proper parser hierarchy: Simulation -> ModelMethod -> NumericalSettings |
| LOGGER.error( | ||
| f'Could not load parser class {self.parser_class_name}', exc_info=e | ||
| ) | ||
| raise |
There was a problem hiding this comment.
Adding a bare raise statement changes error handling behavior. Previously, if the parser class failed to load, the error was logged but execution continued. Now the error is re-raised, which will propagate up the call stack. This is a breaking change in error handling that's unrelated to the PR's purpose of fixing annotation contamination. If this is intentional, it should be mentioned in the PR description under breaking changes.
| model_method.DFT.m_def, | ||
| XML_KEY, | ||
| '.parameters.separator[?"@name"==\'electronic\']', | ||
| '.parameters', |
There was a problem hiding this comment.
The JMESPath expression was changed from .parameters.separator[?"@name"=='electronic'] to .parameters. This removes filtering for the specific 'electronic' separator section. This appears to be a functional change that could cause the parser to extract different or additional data. If this is intentional to fix a bug, it should be documented in the PR description. Otherwise, this could introduce subtle parsing issues.
| '.parameters', | |
| '.parameters.separator[?"@name"==\'electronic\']', |
| from nomad_simulation_parsers.schema_packages import ( | ||
| vasp, # noqa: F401 - needed to register mapping annotations | ||
| ) |
There was a problem hiding this comment.
Import of 'vasp' is not used.
|
We really need a reloading mechanism that works with metainfo. This is necessary as the annotation can be overwritten by other parsers. |
I think that's a completely valid point. I want to leave the design to your discretion to implement it as you want, so I just put this PR up as a suggested solution without touching your code. The issue (to my understanding) is that the annotations are read in all at once, and then fully wiped after the first parser finishes.[1] My feature suggestion is to provide a list of the keys that should be removed. An empty list then either flushes nothing or everything. [1] Bit more elaboration: firstly, I verified this and pushed an example to the branch
|
Instead of annotating base class properties (numerical_settings.Pseudopotential.name), annotate VASP's inherited properties (Pseudopotential.name). This makes VASP immune to annotation removal from other parsers. When a subclass inherits properties, Python creates separate property objects in the subclass namespace. Annotations on Pseudopotential.name are stored separately from numerical_settings.Pseudopotential.name, preventing contamination. All 42 parser tests now pass.
Purpose
Fixes critical global state contamination issue where mapping annotations were being removed, causing parser failures when multiple parsers run in the same process. This affected VASP tests (required subprocess workaround) and could cause production failures in sequential parsing scenarios.
Scope
Included
reload()calls from all 10 parsersremove_mapping_annotations()calls from Exciting, FHI-aims, and H5MD parsersremove_mapping_annotations()utility functionOut of scope
Context & Links
Related to test isolation issue documented in
VASP_TEST_ISOLATION_FIX.md. Root cause: mapping annotations stored on class-levelm_defobjects are shared globally. Two antipatterns caused contamination:reload(schema_module)creates new class instances, but NOMAD plugin system references originalm_defremove_mapping_annotations()removes annotations from shared global stateAnnotations should persist as static metadata for process lifetime, not be dynamically added/removed per parse.
Reviewer Notes
All parsers now follow VASP's pattern of static annotations. Test carefully that:
Status
Breaking Changes
Dependencies / Blockers
Testing / Validation
Before: VASP test_outcar failed in full suite, required subprocess isolation
After: All tests pass, no workarounds needed