aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
53 stars 78 forks source link

Use proper validators instead of raising errors in `prepare_for_submission()` #922

Open mbercx opened 1 year ago

mbercx commented 1 year ago

Currently a lot of validation is still done in the prepare_for_submission() method, for example:

https://github.com/aiidateam/aiida-quantumespresso/blob/cc29488c9e450bac5af7d28ee3f79ab8bcb01b6a/src/aiida_quantumespresso/calculations/__init__.py#L445-L452

This is bad practise, since it's detrimental to user experience for two reasons:

  1. The user has to run/submit the calculation, which winds up in the Excepted state:
(mlist) mbercx@Marniks-Mac-mini 2023-04-23-Witty-ibrav % verdi process list -ap1
  PK  Created    Process label                 Process State     Process status
----  ---------  ----------------------------  ----------------  ----------------------------------
 105  4s ago     PwBaseWorkChain               ⏹ Finished [301]
 106  3s ago     create_kpoints_from_distance  ⏹ Finished [0]
 110  3s ago     PwCalculation                 ⨯ Excepted        Waiting for transport task: upload

Total results: 3

Report: last time an entry changed state: 3s ago (at 07:53:10 on 2023-04-23)
Report: Checking daemon load... OK
Report: Using 0%% of the available daemon worker slots.
  1. It's not easy for the user to figure out what's going wrong. They need to know about verdi process report, and parse a difficult to read Traceback:
(mlist) mbercx@Marniks-Mac-mini 2023-04-23-Witty-ibrav % verdi process report 110
*** 110: None
*** Scheduler output: N/A
*** Scheduler errors: N/A
*** 1 LOG MESSAGES:
+-> REPORT at 2023-04-23 07:53:10.624476+02:00
 | [110|PwCalculation|on_except]: Traceback (most recent call last):
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/tasks.py", line 91, in do_upload
 |     calc_info = process.presubmit(folder)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/calcjob.py", line 836, in presubmit
 |     calc_info = self.prepare_for_submission(folder)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida_quantumespresso/calculations/__init__.py", line 232, in prepare_for_submission
 |     input_filecontent, local_copy_pseudo_list = self._generate_PWCPinputdata(*arguments)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida_quantumespresso/calculations/__init__.py", line 424, in _generate_PWCPinputdata
 |     raise exceptions.InputValidationError(
 | aiida.common.exceptions.InputValidationError: You cannot specify explicitly the 'celldm' flag in the 'SYSTEM' namelist or card.
 | 
 | The above exception was the direct cause of the following exception:
 | 
 | Traceback (most recent call last):
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/plumpy/processes.py", line 1218, in step
 |     next_state = await self._run_task(self._state.execute)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/plumpy/processes.py", line 567, in _run_task
 |     result = await coro(*args, **kwargs)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/tasks.py", line 499, in execute
 |     skip_submit = await self._launch_task(task_upload_job, self.process, transport_queue)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/tasks.py", line 616, in _launch_task
 |     result = await self._task
 |   File "/opt/homebrew/Cellar/python@3.9/3.9.16/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/tasks.py", line 328, in __wakeup
 |     future.result()
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/utils.py", line 119, in execute_coroutine
 |     result = await coro(future)
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/tasks.py", line 103, in task_upload_job
 |     skip_submit = await exponential_backoff_retry(
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/utils.py", line 187, in exponential_backoff_retry
 |     result = await coro()
 |   File "/Users/mbercx/.virtualenvs/mlist/lib/python3.9/site-packages/aiida/engine/processes/calcjobs/tasks.py", line 93, in do_upload
 |     raise PreSubmitException('exception occurred in presubmit call') from exception
 | aiida.engine.processes.calcjobs.tasks.PreSubmitException: exception occurred in presubmit call

I'm assuming this code is still from before validators were a thing. Let's move all that to validators.

To make these changes bite-sized and easy to review, I suggest we split up the work:

mbercx commented 1 year ago

For the validation of the settings/parameters, we should also decide on https://github.com/aiidateam/aiida-quantumespresso/issues/121. I am tentatively in favor of enforcing capitalization of the top-level keys, with lowercase second-level keys for the parameters. Having to deal with both in the code will make it messy, unless we adapt the capitalization on the fly. This is also a possibility, but then we should at least raise a warning.

unkcpz commented 1 year ago

Having to deal with both in the code will make it messy, unless we adapt the capitalization on the fly. This is also a possibility, but then we should at least raise a warning.

This is what I love, having a warning but supporting using lower-case for namelist.