workchain step called run

[BeZie]

Describe the bug

If a step in aworkchain is called run(self), the workchain will not work probably e.g. will finish after this step. Workaround: do not call the step run :)

[sphuber]

Thanks for the report @BeZie . This is actually not a bug, so I have relabeled it. The run method is a method of the Process class, which is also the base class of the WorkChain and it defines how all processes should be run. If this is overridden, it won't work anymore obviously. For now there is no way around this, other than to make sure the documentation clearly state that run cannot be used as an outline step method name. Maybe we can think to change the base method to _run for example, to prevent this accidental override, but this will have to be done in plumpy.

[greschd]

This applies not only to run, but all the methods that are used internally by the WorkChain (e.g. load_instance_state, to name just one example).

I think simply adding an underscore doesn't really protect from this, and adding a more complicated prefix (like _aiida_internal) makes the internal code quite ugly - but that may be acceptable.

As an alternative, could we add a check to spec.outline that warns / raises if any of the internal methods are used as step names? That list could probably be obtained programmatically - although it's also important that this list should not change (because it can break existing workchains).

[sphuber]

Very good point @greschd and actually that shows that prefixing it with something like _aiida_internal is not really an option. There are quite a number of methods, that when overridden, will have unintended consequences for the execution of the workchain. I like the idea of warning when an outline method conflicts with an existing method, but not sure how to do this. You would have to check whether any method in the outline corresponds to an existing method of the class, but if it is already implemented on the workchain implementation, this will be "hidden". Not sure if it is possible to get all methods, excluding those that are implemented on the class itself, so only those of the bases. The alternative of maintaining some hard-coded list of non-overridable methods is really not an alternative at all, I would say.

[greschd]

The problematic methods will always be those of the "base" WorkChain though, right? If some intermediate classes between aiida.engine.WorkChain and the "top-level" class define other methods, I don't think we can (or should) catch that - we don't know if that's supposed to be a step or not.
So we could get the list of methods from aiida.engine.WorkChain, and use that as a black-list.
I do feel it's important to somehow make sure we don't inadvertently add more methods to that "black-list" though. If we both hard-code the list and get it programmatically, we could check inside AiiDA that the two lists match. Of course we could still add "internal" methods, but this would make it more explicit that this is a potentially backwards-incompatible change - and we can think twice about naming of these methods.

[sphuber]

The problematic methods will always be those of the "base" WorkChain though, right?

Nope, also from its base class Process. Imagine defining an outline method out. Now you are no longer able to register outputs with self.out. This is not a WorkChain method, but a generic Process one. The Process class has tens of methods that are important that should not be overridden, so maintaining this as a blacklist is cumbersome and error prone, I feel.

If some intermediate classes between aiida.engine.WorkChain and the "top-level" class define other methods, I don't think we can (or should) catch that - we don't know if that's supposed to be a step or not.

Here I agree, we should not police if a subclass is overriding something of another work chain subclass.

[greschd]

The problematic methods will always be those of the "base" WorkChain though, right?

Nope, also from its base class Process.

Yup, gotcha - I meant WorkChain and everything above it.

Imagine defining an outline method out. Now you are no longer able to register outputs with self.out. This is not a WorkChain method, but a generic Process one. The Process class has tens of methods that are important that should not be overridden, so maintaining this as a blacklist is cumbersome and error prone, I feel.

I agree on it being cumbersome, but we can make it not error prone by comparing against the generated list. Since AiiDA pins its plumpy dependency, I also think that shouldn't be an issue.

The question here is whether we want to take the burden of maintaining this list ourselves - and hopefully enforcing some restraint in adding/blacklisting new methods, or shift that down to the plugin developer who might have their workchains broken because they used a method name that later gets used by the WorkChain (or above).

[sphuber]

This was solved in #5779 where important public methods of the Process class are protected and raise an exception when overwritten in subclass