-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Engine: Do not call serializer for None
values
#5694
Conversation
I added two commits for this PR. Strictly speaking, only the first is necessary to fix the immediate bug, and I think that is the correct fix. But the second commit by itself would have also fixed the immediate cause for the bug in question, however, it would not be as general a fix. I still think that it makes sense to have |
@sphuber thanks!
Do you mean either one of the commits can solve the issue?
I am not sure I understand this, is this addressed by the first commit? |
What I mean is that the current situation is: if a
This is the case for So there are two approaches to fix this:
So the second commit just fixes the problem for one specific serializer, whereas the first commit addresses the problem for all potential serializers. I am just not sure if there are serializers out there now that somehow rely on |
In my opinion, I think serializing |
Thanks @unkcpz . Unless there is anyone else that has a strong opinion, I will then remove the second commit and merge this by Friday. |
a9db671
to
d752987
Compare
@unkcpz I have removed the second commit. Could you review and approve please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have one minor request for the docstring. I assume it is my language issue, so I approve it.
Any port that defines a `serializer`, will have that serializer invoked when a value is passed to this port. The invokation was correctly skipped if no serializer was defined or if the value already was a `Data` node (in which case serialization is no longer needed), but it was also called when this value is `None`. It arguably doesn't make sense to serialize a value that has not been defined. This behavior would cause problems for processes that define a port with a serializer but that also accept `None`. In this case the serializer would nevertheless be called and, in the case of the `to_aiida_type` serializer for example, would except.
d752987
to
1f5ffa4
Compare
Fixes #5693
Any port that defines a
serializer
, will have that serializer invokedwhen a value is passed to this port. The invokation was correctly
skipped if no serializer was defined or if the value already was a
Data
node (in which case serialization is no longer needed), but itwas also called when this value is
None
.It arguably doesn't make sense to serialize a value that has not been
defined. This behavior would cause problems for processes that define a
port with a serializer but that also accept
None
. In this case theserializer would nevertheless be called and, in the case of the
to_aiida_type
serializer for example, would except.