♻️ REFACTOR: move basic parsing into `BaseParser` class

[mbercx]

Fixes #750

Large refactoring of the basic parsing features to deduplicate code and rely on regular expressions via re instead of iterations over lines. The main changes are:

• The Parser class is renamed to BaseParser class to avoid confusion with the aiida.core Parser class.
• The BaseParser._retrieve_parse_stdout method can be used by all subclasses to obtain the parser data and logs from retrieving and parsing the stdout.
• The BaseParser.parse_stdout class method provides the basic parsing features that see if a job has completed and gets the code version and wall time. Subclasses can override this method to add additional stdout parsing. The goal would also be that users can e.g. use NebParser.parse_stdout(stdout) to parse the contents of an output file outside of the scope of AiiDA nodes.
• The error and warning messages are defined as class attributes. Base error messages are defined on the BaseParser class, which can be extended on the subclasses via the class_[error,warning]_map attributes.
• Shared exit codes are moved to the NamelistsCalculation spec from that of the subclasses.

[mbercx]

Some timing information for a very long aiida.out file (~140k lines) without errors:

• Old parse_output_base:

14.4 ms ± 297 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

• New parse_output_base:

10.7 ms ± 125 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

and a very short (131 lines) file with errors:

• Old parse_output_base:

11.4 µs ± 262 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

• New parse_output_base:

23.2 µs ± 271 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

So for large files the new parser is actually faster, but for small files the parsing is slower. We're taking microseconds here though, so I don't think this is a massive issue? At least I think it's worth the simplified code.

[mbercx]

@sphuber currently the PwParser and CpParser don't reuse the base parsing methods yet, but these are rather large and messy parsers I'd like to clean up in separate PRs for v4.0.

[mbercx]

@sphuber she's almost ready, just a bit more cleaning. I would also:

1. Remove the v. for the code versions.
2. Remote the wall_time. It's a useless string and there is wall_time_seconds.

[mbercx]

@sphuber ok, I refactored my initial refactor quite a bit, so perhaps this needs a full review again, apologies. It was also quite a bit of work to fix all the merge conflicts after rebase, so I would now really push to get this PR in.

A couple of quick notes that I decided upon:

• The code version in the output_parameters shouldn't have a v. prefix. It's redundant.
• The wall_time output is similarly redundant and less useful than wall_time_seconds.

Now a more elaborate design discussion on the changes here and TODO's for parsers.

Keeping the parse method readable

Understanding what the parser parses when and which exit codes it returns at what point is difficult with many of the parser classes. We want to make sure that:

1. The "best" exit code is returned, i.e. the one that is most specific or allows for useful error handling.
2. As much data as possible is parsed and added to the outputs of the calculation job.

To improve clarity and make these two requirements possible, I think the parse method should:

1. Not contain any actual parsing. Any real parsing should happen in a separate method.
2. Instantiate a single logging container that is passed through these parsing methods.
3. Take care of decisions on when to return which exit code.
4. Output data at the appropriate time.

Note that this PR does not fully implement these ideas for all parser classes. It's already big enough as it is. But it already shows how this would be possible for e.g. the open_grid.x parser.

Other improvements

• Exit code registry: Although this PR already make an effort of making sure exit codes are consistent across the calculation job classes, it's still tricky to keep track of all the exit codes they are used any why they are there or have a certain number. I think we should create an (automated) exit code registry in the documentation where we keep track of all this. Although two calculation jobs can in principle have the same exit code for different issues, we should probably also avoid this.

• Stop using strings for exit codes: Now we use strings to keep track of the exit codes. I think this is a bit fragile and doesn't allow for nice features like tab-completion. It would be better to have a different approach, but I'd have to think a bit on how to do this.

• PhCalculation and PpCalculation aren't subclasses from NamelistCalculation, and hence have to redefine the basic exit codes. So do PwCalculation and CpCalculation. There may be reasons to treat these calculation jobs differently, but we should investigate if we can't further consolidate this calculation job hierarchy.

[mbercx]

Because of the size of the PR, and the fact that even though the tests pasts it can definitely break things, perhaps:

• @PNOGillespie @superstar54 maybe you can have a look a the XSpectra parsing?
• @qiaojunfeng can you have a look at the open_grid.x parsing changes?

[qiaojunfeng]

* @qiaojunfeng can you have a look at the `open_grid.x` parsing changes?

Had a quick look and seems good to me! Anyhow open_grid is very simple so if the tests passed then I wouldn't worry.

[PNOGillespie]

Hi @mbercx, I'm running some live-fire tests of XSpectra to check for issues. Tests with the XspectraCrystalWorkChain appear to be fine and the output_parameters are as expected, I'm now setting up and re-running tests of the XSpectra-specific error codes to check those too.

[PNOGillespie]

@mbercx. To update the previous comment: I've tested all the XSpectra-specific exit codes, and they all work fine as well. The WorkChains themselves also rely on information in the output_parameters so, since the WorkChains run without issues, I will say that we're good for XSpectra-related stuff.

[mbercx]

Great, thanks @PNOGillespie and @qiaojunfeng! I guess now she just needs a proper review, @sphuber. 😉

[sphuber]

@mbercx did you want to get this merged before the release? If so, is there anything to be done from your side? Otherwise I will approve. We can deal with the open points later. I also would like to have this done so I can finalize my refactor PR for PwParser.

[mbercx]

@mbercx did you want to get this merged before the release? If so, is there anything to be done from your side? Otherwise I will approve. We can deal with the open points later. I also would like to have this done so I can finalize my refactor PR for PwParser.

I've opened https://github.com/aiidateam/aiida-quantumespresso/issues/953 to keep track of the discussions that are left here. I think it makes sense to simply reconsider these decisions when integrating the BaseParser in the PwParser, we'll have a clearer idea of what to do during this process, I think.

[mbercx]

@sphuber it seems the autoapi extension doesn't always play nice with type hints. To avoid going down this rabbit hole here, I simply added the problematic classes to nitpick_ignore. I'll probably look into using autoapi2 soon anyways:

https://pypi.org/project/sphinx-autoapi2/

[sphuber]

Thanks @mbercx let's get this merged

[yakutovicha]

Hi 👋

Just wondering, are you guys planning on making a major release after this PR is merged?

There were API changes, as far as I can see.

[sphuber]

We were getting close to releasing v4.4.0: https://github.com/aiidateam/aiida-quantumespresso/pull/951 @mbercx want to finish that one up and release it?

[mbercx]

want to finish that one up and release it?

Yessir! Sorry for the slow response.

There were API changes, as far as I can see.

The internal parsing API was changed quite a bit, but I think the vast majority of the changes are backwards incompatible. Looking at the data regression files for the tests, the only changes in parsing output is the code_version and wall_time_seconds/wall_time. Which API changes are you worried about breaking e.g. the QEapp?

[yakutovicha]

Which API changes are you worried about breaking e.g. the QEapp?

Mostly about the pp parser (that is the one I noticed). The parse_gaussian method, for instance. Since the name doesn't start with an underscore, it is part of the public API of the class, strictly speaking.

I didn't closely look at the PR, but there might be others.

However, it depends on how closely you want to follow the semantic versioning principles.

[mbercx]

However, it depends on how closely you want to follow the semantic versioning principles.

Yeah, that's fair. Perhaps we should be clearer in defining the public API. I don't expect users to use the Parser classes directly, since in many cases you need to parse from a node (which perhaps we should change over time, but then you definitely break the API).

The parser classes are also quite messy, and we've been working on improving that. Fixing this technical debt would be more important than strictly maintaining backwards compatibility for a part of the API that I don't think users are relying on directly.