Process: Allow None for input ports that are not required
#5722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sphuber
force-pushed
the
feature/5720/allow-none-for-non-required-ports
branch
from
22df578
to
b747f05
Compare
unkcpz
requested changes
Nov 2, 2022
tests/engine/test_process.py
Outdated
| from aiida.manage import get_manager | ||
|
|
||
| runner = get_manager().get_runner() | ||
| process = instantiate_process(runner, TestNotRequiredNoneProcess, not_required=None) |
There was a problem hiding this comment.
I think the process function with input parameter defaulting to None is not covered by this test, correct? Can you add to nightly tests which are exactly the process function manifest the case?
There was a problem hiding this comment.
That is already being tested:
Here we are adding the same behavior for any other process, such as
CalcJob and WorkChain.
There was a problem hiding this comment.
Ah, I see. The FunctionProcess is subclass of Process I thought this PR is going to fix the bug introduced by the previously added feature for None supported input.
It is all good to me! Thanks!
unkcpz
approved these changes
Nov 2, 2022
The tests for the Sphinx extension for process specifications worked using a direct comparison of the parsed XML file. Although this worked fine, we have the `pytest-regressions` plugin for this which handles this automatically, including a useful switch `--force-regen` to regenerate the reference file if it is supposed to change.
sphuber
force-pushed
the
feature/5720/allow-none-for-non-required-ports
branch
from
b72c237
to
9bbc1dc
Compare
Python functions and process functions accept `None` for arguments
that define that as a default without any issues. That is to say,
the following works just fine:
from aiida.engine import calcfunction
@calcfunction
def accepts_none(a=None):
pass
accepts_none()
accepts_none(None)
For a `Process`, however, `None` is not accepted even if the port is not
required. This can be annoying for users because when writing wrappers
around a process launcher, they will have to manually check for one of
the inputs being `None` and remove it before submitting the input
dictionary.
Here we update the `InputPort` constructor to automatically add the
`NoneType` to the `valid_type` argument if the port is marked as
`required=False`. This way, non-required ports will accept `None`
without raising an exception during input validation.
sphuber
force-pushed
the
feature/5720/allow-none-for-non-required-ports
branch
from
9bbc1dc
to
fa9fd9e
Compare
|
@sphuber unfortunately I think this change has had some unintended backwards-incompatible side effects. When trying to run a $ verdi process report 12099
2022-11-11 16:33:57 [4372 | REPORT]: [12099|PwBaseWorkChain|on_except]: Traceback (most recent call last):
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/process_states.py", line 228, in execute
result = self.run_fn(*self.args, **self.kwargs)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/processes/workchains/workchain.py", line 252, in _do_step
finished, stepper_result = self._stepper.step()
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/workchains.py", line 295, in step
finished, result = self._child_stepper.step()
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/workchains.py", line 528, in step
finished, result = self._child_stepper.step()
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/workchains.py", line 295, in step
finished, result = self._child_stepper.step()
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/workchains.py", line 246, in step
return True, self._fn(self._workchain)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/processes/workchains/restart.py", line 197, in run_process
node = self.submit(self.process_class, **inputs)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/processes/process.py", line 523, in submit
return self.runner.submit(process, *args, **kwargs)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/runners.py", line 183, in submit
process_inited = self.instantiate_process(process, *args, **inputs)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/runners.py", line 169, in instantiate_process
return instantiate_process(self, process, *args, **inputs)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/utils.py", line 64, in instantiate_process
process = process_class(runner=runner, inputs=inputs)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/state_machine.py", line 194, in __call__
inst.transition_to(inst.create_initial_state())
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/state_machine.py", line 336, in transition_to
self.transition_failed(initial_state_label, label, *sys.exc_info()[1:])
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/state_machine.py", line 352, in transition_failed
raise exception.with_traceback(trace)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/state_machine.py", line 321, in transition_to
self._enter_next_state(new_state)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/state_machine.py", line 383, in _enter_next_state
self._fire_state_event(StateEventHook.ENTERING_STATE, next_state)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/state_machine.py", line 300, in _fire_state_event
callback(self, hook, state)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/processes.py", line 329, in <lambda>
lambda _s, _h, state: self.on_entering(cast(process_states.State, state)),
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/processes/process.py", line 405, in on_entering
super().on_entering(state)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/processes.py", line 675, in on_entering
call_with_super_check(self.on_create)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/utils.py", line 29, in call_with_super_check
wrapped(*args, **kwargs)
File "/home/mbercx/envs/aiida-dev/code/aiida-core/aiida/engine/processes/process.py", line 395, in on_create
super().on_create()
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/base/utils.py", line 16, in wrapper
wrapped(self, *args, **kwargs)
File "/home/mbercx/.virtualenvs/aiida-dev/lib/python3.8/site-packages/plumpy/processes.py", line 732, in on_create
raise ValueError(result)
ValueError: Error occurred validating port 'inputs.settings': value 'settings' is not of the right type. Got '<class 'aiida.common.extendeddicts.AttributeDict'>', expected '(<class 'aiida.orm.nodes.data.dict.Dict'>, <class 'NoneType'>)'
2022-11-11 16:33:57 [4373 | REPORT]: [12099|PwBaseWorkChain|on_terminated]: remote folders will not be cleanedThe problem is that the I was looking into fixing this in the EDIT: Actually, it's the |
|
Thanks for letting us know @mbercx . I think this was actually really an inaccuracy that was anyway hiding in the elif port.valid_type == orm.Dict and isinstance(value, dict):to valid_types = port.valid_type if isinstance(port.valid_type, (list, tuple)) else (port.valid_type,)
...
elif orm.Dict in valid_types and isinstance(value, dict):Do you really think there is other code out there that might break due to this? |
|
Well, the QE plugin also does ^^ As I said in #5757, I'm not sure if there is a lot of other plugins that rely on checking the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5720
Python functions and process functions accept
Nonefor argumentsthat define that as a default without any issues. That is to say,
the following works just fine:
For a
Process, however,Noneis not accepted even if the port is notrequired. This can be annoying for users because when writing wrappers
around a process launcher, they will have to manually check for one of
the inputs being
Noneand remove it before submitting the inputdictionary.
Here we update the
InputPortconstructor to automatically add theNoneTypeto thevalid_typeargument if the port is marked asrequired=False. This way, non-required ports will acceptNonewithout raising an exception during input validation.