feat(parsers): add write_legacy_and_new utility helper - #223
Conversation
|
I know this is not optimal but we really do not want to mix the two parsers as it would require more work later to disentangle them. Also, the file parsers are separate now and the legacy is not necessarily kept up-to-date which means that if we supply it with data not compatible with legacy schema it could be problematic. Do you want to do this to all the parsers? I do not know the timeline here for the complete removal of the legacy parsers but I will discuss this with area c if the performance gain is worth the hassle. |
|
It would be good to do these for the following three: 
The others are all around 100k or less so that's fine. For FH-aims for example, let's say if it's a 20s saving, that would still be over 5000hrs of processing time saved when it comes to re-processing so that's quite significant |
Okay. can you maybe also implement the same for gaussian and vasp also in this pr? I will have closer look this week. I guess the reprocessing can wait. I will also have to verify for these 3 parsers that reusing the new file parser is not problematic. |
|
I don't see any gaussian parser yet. The VASP parser is a bit different, it uses a different xml parser. The outcar parsing could be optimised but I can do that in a different PR |
I would suggest implementing separately for the 3 parsers. FHIaims now and we can continue the testing and start the targeted reprocessing The VASP parser has not been assessed fully and may need significant work, we can then address the needed utilities, after the approach is validated for FHIaims...note that during the VASP reprocessing we not only need to reprocess the existing entries but also upload the full Alexandria database The Gaussian parser, as you mentioned, has not been started. We are starting development now, so we will need more time until that one is ready |
c1775b0 to
8e081e2
Compare
|
The orca test is failing on test oasis branch too! @ladinesa ready otherwise |
f6178e6 to
cf4e0df
Compare
This has been resolved yesterday. |
8e081e2 to
6a4e21a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The current VASP execution order contradicts the PR intent and is associated with newly skipped failing tests, indicating unresolved functional issues.
Pull request overview
This PR introduces a utility to run a legacy parser and a new ArchiveWriter sequentially, enabling reuse of a parsed text-parser instance to avoid duplicate file scans, and updates multiple parsers/tests/dependencies to support a transition period where legacy parsers are still executed.
Changes:
- Add
write_legacy_and_new(...)helper to run legacy parsing + new writer in sequence and optionally pass through a reused parsed text parser. - Enhance the FHI-aims pipeline to reuse the parsed text parser and extend legacy quantities to support next-gen mappings.
- Temporarily invoke legacy parsers in several parser implementations, update dependencies (new plugin repos), and adjust/skip some tests.
File summaries
| File | Description |
|---|---|
| uv.lock | Updates lockfile resolution (notably numpy dependency entries) and adds additional packages/plugins. |
| pyproject.toml | Adds git dependencies for parser-plugin repos needed by the transitional legacy-parser execution. |
| tests/parsers/test_vasp_parser.py | Skips workflow-related VASP tests due to legacy/new ordering issues (currently contains a bug in skip reason construction). |
| tests/parsers/test_h5md_parser.py | Adjusts imports, skips MD test pending a normalizer fix, and comments out several assertions (reducing coverage). |
| src/nomad_simulation_parsers/parsers/utils/general.py | Adds write_legacy_and_new helper to coordinate legacy parse + writer execution and pass through parsed_text_parser when supported. |
| src/nomad_simulation_parsers/parsers/fhiaims/parser.py | Integrates write_legacy_and_new, extends legacy out_parser quantities, and wires parsed text parser reuse into the archive writer. |
| src/nomad_simulation_parsers/parsers/fhiaims/out_parser.py | Updates structure regex patterns for robustness. |
| src/nomad_simulation_parsers/parsers/vasp/parser.py | Runs the new writer and then the legacy parser (currently opposite of the PR’s stated intent). |
| src/nomad_simulation_parsers/parsers/quantumespresso/parser.py | Adds legacy parser execution before the new writer selection/writing. |
| src/nomad_simulation_parsers/parsers/yambo/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/wannier90/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/phonopy/parser.py | Adds legacy workflow parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/orca/parser.py | Adds legacy parser execution before continuing the newer parsing flow. |
| src/nomad_simulation_parsers/parsers/octopus/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/lobster/parser.py | Adds legacy workflow parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/h5md/parser.py | Adds legacy atomistic parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/gromacs/parser.py | Adds legacy atomistic parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/gpaw/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/exciting/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/crystal/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/ams/parser.py | Adds legacy parser execution before invoking the new archive writer. |
| src/nomad_simulation_parsers/parsers/abinit/parser.py | Adds legacy parser execution before invoking the new archive writer. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _extend_legacy_out_parser(legacy_out_parser: Any) -> None: | ||
| """Extend the legacy out_parser with next-gen simulation quantities.""" | ||
| from nomad_simulation_parsers.parsers.fhiaims.out_parser import ( # noqa: PLC0415 | ||
| FHIAimsOutFileParser, |
There was a problem hiding this comment.
An issue that I see here is that we are trying to graft the legacy parser which is in non-line_parsing mode with a parser modified for line parsing. We can switch to the old non-line parsing but this has issues with large files. Or we switch line parsing for large files and not do the grafting.
There was a problem hiding this comment.
Dual-write always runs the legacy parser first, so that mmap pass is mandatory for large files too. Skipping the graft and line-parsing new parser on its own would not avoid that cost. So along as we have to keep populating the runsection there's no way to avoid that cost unless you want to switch the old parser to use the line parser too.
There was a problem hiding this comment.
yes that was on my mind too, I was really just hesitant to t touch the legacy parser. I will update it to use line parser.
There was a problem hiding this comment.
For the legacy one doesn't the regex change fix it? Do we need to use a line parser at all? Or how big of a file does it have to be for that to blow up
There was a problem hiding this comment.
It has a different regex as it does not match a block but lines. Anyway I created an issue there. will simply copy the line parser here and make sure it works.
There was a problem hiding this comment.
I will have to check if your recent fix atually fixes the large file issue.
There was a problem hiding this comment.
It drops peak RSS by 80% for the largest FHaims files I have locally
There was a problem hiding this comment.
I can confirm that your fix actually worked for the large file. I can do the change in develop then you rebase.
| archive_handler.annotation_key = fhiaims.TEXT_DOS_KEY | ||
| out_parser.convert(archive_handler, remove=False) | ||
| has_dos = bool( | ||
| out_parser.data.get('total_dos_files') |
There was a problem hiding this comment.
total_dos_files actually live in the the individual sub-sections: full_scf, geometry_optimization and molecular_dynamics not in root.
| parser = FHIAimsArchiveWriter() | ||
| parser.annotation_key = fhiaims.TEXT_GW_KEY | ||
| parser.write(self.mainfile, gw_archive, self.logger) | ||
| parser.write( |
There was a problem hiding this comment.
i suggest not changing the function arguments for write.
| return _log(function) if function else _log | ||
|
|
||
|
|
||
| def write_legacy_and_new( # noqa: PLR0913, PLR0917 |
There was a problem hiding this comment.
i would drop this utility since in most cases, you have several of these text parsers. inline this in fhiaims and generalize this later if the need arises
|
@blueraft please rebase, @ndaelman-hu please have a closer look at the alias mapping. |
cf4e0df to
d791a2c
Compare
- Provide sequential execution helper for legacy parser and new archive writer - Reuse parsed text parser instance across both stages to eliminate duplicate file scans
8827d05 to
f485333
Compare
FHI-aims mapping — semantic faithfulness vs
|
| Parser mapping (source → target) | #476 reference (runschema → target) | #476 status | Faithful |
|---|---|---|---|
.version → Program.version |
run.program.version → Simulation.program.version |
Mapped | ✓ |
.threshold_change → SelfConsistency.threshold_change |
method.scf.threshold_energy_change → SelfConsistency.threshold_change |
Partial (unified) | ✓ |
.n_max_iterations → SelfConsistency.n_max_iterations |
method.scf.n_max_iteration → …n_max_iterations |
Mapped | ✓ |
.name → SelfConsistency.name |
method.electronic.method / SCF identity |
Partial | ✓ |
get_functional_key(.controlInOut_xc) → XCFunctional.functional_key |
method.dft.xc_functional.name → XCFunctional.functional_key |
Partial (canonical alias) | ✓ |
.@ → KSpace.k_mesh |
method.k_mesh → KSpace.k_mesh[] |
Mapped | ✓ |
.k_grid → KMesh.grid |
method.k_mesh.grid → KMesh.grid |
Mapped | ✓ |
get_k_offset_with_default(.k_offset) → KMesh.offset |
method.k_mesh.offset → KMesh.offset |
Mapped | ✓ |
get_gw_flag(.gw_flag) → GW.type |
method.gw.type → GW.type |
Mapped | ✓ |
.structure.positions → ModelSystem.positions |
system.atoms.positions → ModelSystem.positions |
Mapped | ✓ |
.lattice_vectors → Representation.lattice_vectors |
system.atoms.lattice_vectors → Representation.lattice_vectors |
Mapped | ✓ |
get_periodic_boundary_conditions(.@) → Representation.periodic_boundary_conditions |
system.atoms.periodic → …periodic_boundary_conditions |
Mapped | ✓ |
get_topology_labels(.structure.labels) → AtomsState.chemical_symbol |
system.atoms.labels → AtomsState.chemical_symbol |
Mapped | ✓ |
get_energies(.@) → Outputs.total_energies (.value) |
calculation.energy.total.value → Outputs.total_energies[].value |
Mapped | ✓¹ |
.components → TotalEnergy.contributions |
calculation.energy.contributions[].value → total_energies[].contributions[].value |
Mapped | ✓¹ |
get_forces(.@) → Outputs.total_forces |
calculation.forces.total.value → Outputs.total_forces[].value |
Mapped | ✓ |
get_eigenvalues(.eigenvalues) → Outputs.electronic_eigenvalues |
calculation.eigenvalues.energies → electronic_eigenvalues[].value |
Mapped | ✓ |
get_band_structures(.eigenvalues) → Outputs.electronic_band_structures |
band_structure_electronic.segment.energies → electronic_band_structures[].value |
Mapped | ✓ |
get_band_gaps(.@) → Outputs.electronic_band_gaps |
calculation.band_gap.value → electronic_band_gaps[].value |
Mapped | ✓ |
.spin_channel → ElectronicBandGap.spin_channel |
calculation.band_gap.index → electronic_band_gaps[].spin_channel |
Partial | ✓ |
get_dos(.total_dos_files, …) → Outputs.electronic_dos (.value) |
calculation.dos_electronic.total → electronic_dos[].value |
Mapped | ✓² |
.projected_dos → ElectronicDensityOfStates.projected_dos |
dos_electronic.atom_projected/orbital_projected → projected_dos[].value |
Mapped | ✓² |
.energies → Energy2.points |
dos_electronic.energies → electronic_dos[].energies.points |
Mapped | ✓ |
get_scf_steps(.@) → Outputs.scf_steps |
calculation.scf_iteration[] → Outputs.scf_steps |
Partial | ✓ |
.delta_energies_total → SCFSteps.delta_energies_total |
scf_iteration.energy.change → scf_steps.delta_energies_total |
Partial | ✓ |
.durations → SCFSteps.durations |
scf_iteration.time_calculation → scf_steps.durations |
Partial | ✓ |
¹ Faithful for the total value and generic contributions — but kinetic/potential energies are routed here too, rather than to their dedicated Outputs.kinetic_energies / potential_energies properties. Flagged as a separate comment below.
² The Outputs DOS section is built by a get_sections mapper whose include=[...] argument lists which parsed keys to carry into the section; in fhiaims.py that list is ['total_dos_files', 'species_projected_dos_files'] — it leaves out atom_projected_dos_files. But the get_dos transformer takes atom_projected_dos_files as one of its inputs, so the atom-projected file list may be filtered out before get_dos runs. Worth confirming atom-projected DOS still reaches it.
|
@ndaelman-hu do you want to change anything? Or should I merge it? |
Kinetic/potential energies are flattened into
|
|
I see, in that case I'll merge this and you can improve those issues in follow up PRs whenever it's ready |
Depends on nomad-coe/electronic-parsers#340