aiidateam / aiida-common-workflows

A repository for the implementation of common workflow interfaces across materials-science codes and plugins
https://aiida-common-workflows.readthedocs.io
MIT License
52 stars 31 forks source link

BigDFT: common relax workchain excepts because of missing pseudo potential file #158

Closed sphuber closed 3 years ago

sphuber commented 3 years ago

Excepts in presubmit:

ValueError: path `/.virtualenvs/aiida/lib/python3.7/site-packages/BigDFT/scripts/psppar/psppar.Si` does not correspond to an existing file

This is on Quantum Mobile with aiida-bigdft==0.2.5. Are these files included in the MANIFEST.in @adegomme ? This is necessary for them to be included in the distribution that is released on PyPI.

adegomme commented 3 years ago

I can confirm that the files are in the pip package, but I wonder if the path is not missing an aiida-bigdft before BigDFT .. is /.virtualenvs/aiida/lib/python3.7/site-packages/aiida_bigdft/BigDFT/scripts/psppar/psppar.Si a file ?

sphuber commented 3 years ago

Actually, /home/max/.virtualenvs/aiida/lib/python3.7/site-packages/BigDFT/scripts/psppar/psppar.Si does exist. It seems that the $HOME expansion may be failing somehow because it is missing /home/max in the path. I wonder what part of the code is responsible for creating the entire path though? Don't think the plugin should have anything to do with it, but then who?

sphuber commented 3 years ago

Ah, I see now. The pseudos are in the BigDFT package itself, and not the Python one. This is probably where the problem lies somewhere. I was looking at the BigDFTCalculation and saw the following:

        # setup pseudopotentials if needed
        if self.inputs.pseudos is not None:
            for filename in self.inputs.pseudos:
                pseudo_filedata = SinglefileData(
                    file=os.path.abspath(filename)).store()
                local_copy_list.append(
                    (pseudo_filedata.uuid, pseudo_filedata.filename, pseudo_filedata.filename))

and that the pseudos are past in the inputs as a List node with strings that are the full path. May I ask why you chose this approach? This is very fragile (and I think has to do with the error I describe in the OP) and not the recommended way in AiiDA I think.

The problem actually lies in the input generator implementation. It uses os.rel to build the filename paths of the pseudos:

https://github.com/aiidateam/aiida-common-workflows/blob/75cc23ed0ab5079cb1a4f5ec566edd042e916ad7/aiida_common_workflows/workflows/relax/bigdft/generator.py#L288

but they can then be relative as shown by the attributes of the pseudos input List node:

(aiida) max@a54d4363604c:/$ verdi node attributes 4092
PK: 4092
{
    "list": [
        "../.virtualenvs/aiida/lib/python3.7/site-packages/BigDFT/scripts/psppar/psppar.Si"
    ]
}

I think we can fix the immediate problem by calling os.path.normpath on the relative paths, turning them into absolute paths. However, I would still recommend to rethink this design of pseudos at some point and maybe consider the approach that other plugins use where the pseudos input is a namespace and the pseudo potentials that should be used are passed in as nodes ( SinglefileData as the most simple case) at runtime.

sphuber commented 3 years ago

Can confirm that normalizing the path in the input generator fixes the immediate problem, although there is another exception now. This one that comes from the LogFile.

*** 1 LOG MESSAGES:
+-> REPORT at 2021-03-10 12:09:31.811655+00:00
 | [4754|BigDFTCalculation|on_except]: Traceback (most recent call last):
 |   File "/home/max/.virtualenvs/aiida/lib/python3.7/site-packages/plumpy/process_states.py", line 225, in execute
 |     result = self.run_fn(*self.args, **self.kwargs)
 |   File "/home/max/codes/aiida-core/aiida/engine/processes/calcjobs/calcjob.py", line 313, in parse
 |     exit_code_retrieved = self.parse_retrieved_output(retrieved_temporary_folder)
 |   File "/home/max/codes/aiida-core/aiida/engine/processes/calcjobs/calcjob.py", line 392, in parse_retrieved_output
 |     exit_code = parser.parse(**parse_kwargs)
 |   File "/home/max/.virtualenvs/aiida/lib/python3.7/site-packages/aiida_bigdft/parsers.py", line 63, in parse
 |     get_abs_path(output_filename))
 |   File "/home/max/.virtualenvs/aiida/lib/python3.7/site-packages/aiida_bigdft/data/__init__.py", line 116, in __init__
 |     self.logfile = path
 |   File "/home/max/.virtualenvs/aiida/lib/python3.7/site-packages/aiida_bigdft/data/__init__.py", line 132, in logfile
 |     self.bigdftlogfile = Logfiles.Logfile(path)
 |   File "/home/max/.virtualenvs/aiida/lib/python3.7/site-packages/BigDFT/Logfiles.py", line 392, in __init__
 |     raise ValueError("No log information provided.")
 | ValueError: No log information provided.
adegomme commented 3 years ago

Thanks a lot for the analysis, I will have a look this afternoon and push a fix asap. I won't be able to attend the meeting though.

adegomme commented 3 years ago

I was trying to reproduce on a quantum mobile docker with 0.2.5 yesterday, but failed to trigger it using cli, what script/command did you use (maybe it was on the vm but I can't have it built, hostname issues are blocking) ? My laptop does not have enough ram to handle a full computation to reach the end, I have to try on a proper machine for the second issue.. (And that means rebuilding, as apparently docker save/scp/import is useless and loses important information to run containers in the process)

sphuber commented 3 years ago

I added a full script in this issue. It includes the docker-compose.yml script that I used and how I run the workflow through the CLI. Let me know if that works for you.

adegomme commented 3 years ago

Thanks, indeed I can now reproduce on my docker build, if the command line is run from / (as done in your script, the only change I made on my attempts yesterday was to move to /home/max first). I will now check how to avoid this.