ExitCode: make the exit message parameterizable through templates
#3824
Conversation
sphuber
force-pushed
the
feature/2644/exit-code-message-template
branch
from
fd93b6d to
6fd925b
Compare
sphuber
force-pushed
the
feature/2644/exit-code-message-template
branch
from
6fd925b to
d3a0e15
Compare
sphuber
force-pushed
the
feature/2644/exit-code-message-template
branch
from
d3a0e15 to
3494c29
Compare
| from aiida.common.extendeddicts import AttributeDict | ||
|
|
||
| __all__ = ('ExitCode', 'ExitCodesNamespace') | ||
|
|
||
| ExitCode = namedtuple('ExitCode', ['status', 'message', 'invalidates_cache']) |
There was a problem hiding this comment.
This is technically a change in interface, because namedtuple exposes all the methods of the underlying tuple. For example, one could do
status, message, invalidates_cache = ExitCode()
I'm not sure if any of these "accidental features" is used anywhere, though.
There was a problem hiding this comment.
Good point. Do you know of a way to make a simple class replicate the exact interface/behavior of a named tuple? If not, we might have to put this in 2.0
There was a problem hiding this comment.
Indeed. Also this functionality is probably not super critical, depending on when a 2.0 release is scheduled.
There was a problem hiding this comment.
Behold, a thing of beauty:
from collections import namedtuple
class ExitCode(namedtuple('_ExitCode', ['status', 'message', 'invalidates_cache'])):
def __call__(self, **kwargs):
pass
ExitCode.__new__.__defaults__ = (0, None, False)
ExitCode(0, 'bla')()
aiida/engine/processes/exit_code.py
Outdated
| When this namedtuple is returned from a Process._run() call, it will be interpreted that the Process | ||
| should be terminated and that the exit status and message of the namedtuple should be set to the | ||
| corresponding attributes of the node. | ||
| class ExitCode: |
There was a problem hiding this comment.
I'm not entirely convinced we should mix the "exit code" and "exit code template" concepts in the same class. On the one hand, this makes it easy in the spec.exit_code call (you just pass on the message) -- but on the other hand, this makes it almost impossible for the code that uses the exit code to figure out which one it is.
Then again I can't come up with a convincing example for when you would need to distinguish this, so this might be fine.
There was a problem hiding this comment.
The template part here only really applies to the message though and as the message really is just a human readable description/alternative of the exit status, I don't think this is a problem. Any machine will only ever just have to look at the status which is "immutable" and should never even look at the message.
There was a problem hiding this comment.
Yeah, by "calling" code I meant where the exit code is returned from.. like, maybe you want to know if you should "format" the exit code or no.
There was a problem hiding this comment.
But, again, I'm not super convinced on this. Since the message has to use named formatting you can always "format" with a dict that has too many keys.
aiida/engine/processes/exit_code.py
Outdated
| self.message = message | ||
| self.invalidates_cache = invalidates_cache | ||
|
|
||
| def __call__(self, **kwargs): |
There was a problem hiding this comment.
Also not completely sure about using __call__ for this - if we want to extend the functionality of the ExitCode class in the future, it would be slightly arbitrary to give __call__ to this method. Maybe evaluate or format?
There was a problem hiding this comment.
Of course this very PR defeats what I am about to say but I don't think we will be adding more functionality to this simple data class. The choice for using __call__ is really for the user interface that it gives where it will be used most often. I think the majority of users never really even instantiate an ExitCode manually. They are defined on the spec through ProcessSpec.exit_code and are retrieved through the WorkChain.exit_codes and Parser.exit_codes namespace properties just before being returned. That is to say, the vast majority of code that will be written with this feature will look like either:
return self.exit_codes.ERROR_WRONG_PARAMETER(parameter='some_key')
or
return self.exit_codes.ERROR_WRONG_PARAMETER.format(parameter='some_key')
We can have a quick under more users to see what syntax is preferred. I would be fine with both if the majority prefers not to use the call.
There was a problem hiding this comment.
In fact maybe the latter anyway reads a bit more pythonic, e.g. what you expect from strings etc. Also, the extra format is not that cluttering and we are not locking down the call by using this instead. I am happy to do the latter.
There was a problem hiding this comment.
I think the latter conveys the intent a bit better - makes it clear that the parameters are there only for formatting the message, not being taken into account in any other way.
But, this might be bike-shedding (sorry for that), and I certainly don't have a strong opinion either way. Both versions will work just fine.
Often one would like to define an exit code for a process where the
general gist of the message is known, however, the exact form might be
slightly situation dependent. Creating individual exit codes for each
slight variation is cumbersome so we introduce a way to parameterize the
exit message. To do this the `ExitCode` had to be changed from named
tuple to a proper class, such that the `__call__` method could be
implemented. This allows a mechanism where calling an exit code with a
parameterized exit message can be concretized. The keyword arguments are
used for the string interpolation and, importantly, a new exit code
instance is returned. Example:
exit_code_template = ExitCode(450, 'the parameter {parameter} is invalid.')
exit_code_concrete = exit_code_template(parameter='some_specific_key')
The `exit_code_concrete` is now unique to `exit_code_template` except
for the message whose parameters have been replaced.
Using an explicit format` method instead of overriding `__call__` conveys the function more clearly and keeps the call method available for potential future uses.
sphuber
force-pushed
the
feature/2644/exit-code-message-template
branch
from
3494c29 to
3ccbcf7
Compare
Changing the `ExitCode` from a namedtuple to a class breaks backwards compatibility since the class won't have the functionalities of a tuple such as expanding an instance in its attributes. By subclassing from the namedtuple definition we keep all that functionality while still being able to implement additional methods, such as in this case `format`. Co-Authored-By: Dominik Gresch <greschd@users.noreply.github.com>
sphuber
force-pushed
the
feature/2644/exit-code-message-template
branch
from
3ccbcf7 to
4ec0662
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3824 +/- ##
===========================================
- Coverage 77.16% 69.19% -7.97%
===========================================
Files 458 458
Lines 33776 33788 +12
===========================================
- Hits 26064 23381 -2683
- Misses 7712 10407 +2695
Continue to review full report at Codecov.
|
|
@greschd this virus outbreak has made me see the light and I agree with your suggestions presented during the bike shedding. One now uses |
Fixes #2644
Often one would like to define an exit code for a process where the
general gist of the message is known, however, the exact form might be
slightly situation dependent. Creating individual exit codes for each
slight variation is cumbersome so we introduce a way to parameterize the
exit message. To do this the
ExitCodehad to be changed from namedtuple to a proper class, such that the
__call__method could beimplemented. This allows a mechanism where calling an exit code with a
parameterized exit message can be concretized. The keyword arguments are
used for the string interpolation and, importantly, a new exit code
instance is returned. Example:
The
exit_code_concreteis now unique toexit_code_templateexceptfor the message whose parameters have been replaced.