WorkChain: Protect public methods from being subclassed

[sphuber]

Fixes #5467

The paradigm of the WorkChain requires a user in an implementation to define the workflow logic through classmethods of the WorkChain subclass. While this gives great flexibility and choice to the user, there is a risk that a user inadvertently chooses a method name that already exists on the WorkChain base class. Usually the super is not called in this scenario and so the functionality is broken.

The typical example is where the user uses the run method as a step in the outline of the WorkChain. The work chain will still run, however, only that one step in the outline is called. Since the logic to continue to the next step in the outline is defined in WorkChain.run, which is overridden and now no longer called, the rest of the work chain is skipped without any warning or error message, leaving the user scratching their head as to what happened.

Here we protect this and other public methods on the WorkChain class to prevent them from being overridden in subclasses. This is accomplished by adding the Protect class as a metaclass. Since the WorkChain already has the metaclass plumpy.ProcessStateMachineMeta, which it inherits from its Process base class, and all metaclasses need to share the same base, Protect also subclasses the ProcessStateMachineMeta class.

The Protect class provides the final classmethod which can be used to decorate a method in the WorkChain class that should be protected. If a subclass implements it, as soon as the class is imported, a RuntimeError is raised mentioning that the method cannot be overridden.

The test test_report_dbloghandler had to be fixed because it actually suffered from the very problem that is being fixed. It used the run method to setup the test, but since the check was never being called, the test always passed, even though the code self._backend in the check is incorrect.

[sphuber]

@danielhollas I think this fix should have saved us some debugging time in #5775 😅

[danielhollas]

@sphuber did you mean to tag @janssenhenning ?

In any case, this is cool Python magic. 👍

I would additionally propose to rename the run method to something less generic so that the potential clash is minimized. Not sure if that has any potential to break things though.

[unkcpz]

@sphuber thanks! Before going into details, there are some github hints in the review panel, like below. What are they, is that indicate some glitches need to be taken care of?

[danielhollas]

Edit, now I understand, the run method is part of public API, so cannot be changed. Sorry for the confusion.

[sphuber]

@sphuber did you mean to tag @janssenhenning ?

I did indeed 😅 apologies.

I would additionally propose to rename the run method to something less generic so that the potential clash is minimized. Not sure if that has any potential to break things though.

Not a bad suggestion, but this would require changes in plumpy for sure. Not sure how trivial this is. Would leave this for a separate discussion.

[sphuber]

@sphuber thanks! Before going into details, there are some github hints in the review panel, like below. What are they, is that indicate some glitches need to be taken care of?

These have been there for a long long time. I am not sure how they got enabled, but I have the feeling this is GHA trying to be clever. It seems to be picking up on exceptions that get raised in our tests, but these tests are doing that on purpose and so that is exactly what we expect.

[unkcpz]

@sphuber thanks!
Can you explain why has to create Protect class rather than using @typing.final directly, I think the reason is you make it a runtime check here. But didn't understand the full trick here.

[unkcpz]

If I understand correctly, the Protect serve two purposes, 1. protect all public methods from ProcessStateMachineMeta, 2. provide final decorator for other workchain class.
I wonder if is it more clear to separate these two purposes by decoupling the decorator and using it in the Protect class?

[sphuber]

Can you explain why has to create Protect class rather than using @typing.final directly, I think the reason is you make it a runtime check here. But didn't understand the full trick here.

Indeed, the typing.final decorator only reports an error during type checking but we need an error to be raised during runtime. Most of the workchain developers will not be running active type checking on their plugins and so we need something more aggressive than typing.final

[sphuber]

If I understand correctly, the Protect serve two purposes, 1. protect all public methods from ProcessStateMachineMeta, 2. provide final decorator for other workchain class.
I wonder if is it more clear to separate these two purposes by decoupling the decorator and using it in the Protect class?

Not quite. The public methods of ProcessStateMachineMeta are not automatically protected from being overwritten. Only those that are explicitly marked by the final decorator are protected. The meta class is necessary to have the check automatically applied as soon as a new class of WorkChain is defined.

[unkcpz]

Thanks for the explanation! All good to me.