`LAMMPSBaseParser`: Fix parsing for nodes with the `script` input

[sphuber]

The BaseLammpsCalculation plugin was recently updated to allow specifying the exact script to run through the script input node. If the default parser is used in this case, which is the LAMMPSBaseParser, a non-zero exit code would always be returned because some of the output files that the parser wouldn't be there. These output files are normally written by the script that the plugin itself would create, but if a complete script is taken from inputs, it cannot be guaranteed that the script will produce these same outputs.

The parser is updated to only return an exit code if an expected output file is not in the retrieved files if the script input was not defined. In addition, the total_wall_time key, if parsed by the log file parser, is parsed and converted to seconds and added as the total_wall_time_seconds key.

[codecov[bot]]

Codecov Report

Merging #60 (a8590b5) into develop (bb495bc) will decrease coverage by 0.04%. The diff coverage is 82.75%.

@@             Coverage Diff             @@
##           develop      #60      +/-   ##
===========================================
- Coverage    90.26%   90.23%   -0.04%     
===========================================
  Files           32       32              
  Lines         2414     2426      +12     
===========================================
+ Hits          2179     2189      +10     
- Misses         235      237       +2     

Flag	Coverage Δ
pytests	90.23% <82.75%> (-0.04%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida_lammps/parsers/lammps/lammps_parser.py	81.66% <82.75%> (+0.41%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sphuber]

I think it looks good, just remove the print statement in the parser when dealing with the global variables.

Done.

any idea why the previous tests did not catch this issue?

Well, as far as I can tell, this parser was not tested whatsoever. I think that this parser was maybe the original one (together with BaseLammpsCalculation) and they were never tested. When you started to refactor things, you added tests, but only for the new plugins that you added (lammps.force, lammps.md, etc.).

We should maybe think how to harmonize all of these, because now there seems to be some duplication.

[JPchico]

any idea why the previous tests did not catch this issue?

Well, as far as I can tell, this parser was not tested whatsoever. I think that this parser was maybe the original one (together with BaseLammpsCalculation) and they were never tested. When you started to refactor things, you added tests, but only for the new plugins that you added (lammps.force, lammps.md, etc.).

Well that explains it. It is very nice that you added a test for the parser. I think that the plan was to do that sometime ago, but well clearly I must have forgotten to do it. I'll look and see if more tests are needed to make sure that no funky business can happen.

We should maybe think how to harmonize all of these, because now there seems to be some duplication.

I agree 100% I think that the idea that @chrisjsewell had was that we should leave the old functions in case somebody needs them. But I think it might actually be best to remove them, or mark them for deprecation and make the test more modular. Since we end up having a lot of duplication and it is easy to miss something.

So I'm all for removing the old parsers and calcjobs and keep the new ones and just run with that.

[sphuber]

So I'm all for removing the old parsers and calcjobs and keep the new ones and just run with that.

That'd be fine with me as well, as long as I can still port over the functionality that I still added that allow adding a complete script as input and have the wall time seconds parsed. But if you are ok with that as well, then I am fine with opening a PR for that.

[sphuber]

@JPchico did anything still needed to be done for this PR?