Skip to content
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

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Oct 7, 2022

Fixes #5693

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.

@sphuber
Copy link
Contributor Author

sphuber commented Oct 7, 2022

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 to_aiida_type not choke on None.

@unkcpz
Copy link
Member

unkcpz commented Oct 11, 2022

@sphuber thanks!

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

Do you mean either one of the commits can solve the issue?

I still think that it makes sense to have to_aiida_type not choke on None.

I am not sure I understand this, is this addressed by the first commit?

@sphuber
Copy link
Contributor Author

sphuber commented Oct 12, 2022

Do you mean either one of the commits can solve the issue?

What I mean is that the current situation is: if a serializer is defined on an InputPort, that serializer is called, even if the value of the port specified is None. If the serializer does not know that it can receive None and does not deal with it, it might except.

I am not sure I understand this, is this addressed by the first commit?

This is the case for to_aiida_type, which raises when it gets a None value.

So there are two approaches to fix this:

  1. Simply have to_aiida_type accept None and simply return None (this is the second commit)
  2. We think that this is a broader problem and a serializer should simply never be called when the value is None (this is the first commit)

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 None being passed. I doubt it to be honest, but we would be changing that behavior.

@unkcpz
Copy link
Member

unkcpz commented Oct 12, 2022

In my opinion, I think serializing None makes not much sense. The None set in the argument is more like a placeholder than the data to be serialized. If 1st commit already solve the issue I think it is enough and it should not go further and try to serialize it.

@sphuber
Copy link
Contributor Author

sphuber commented Oct 12, 2022

Thanks @unkcpz . Unless there is anyone else that has a strong opinion, I will then remove the second commit and merge this by Friday.

@sphuber
Copy link
Contributor Author

sphuber commented Oct 13, 2022

@unkcpz I have removed the second commit. Could you review and approve please?

Copy link
Member

@unkcpz unkcpz left a 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.

aiida/engine/processes/ports.py Outdated Show resolved Hide resolved
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.
@sphuber sphuber merged commit cc4fa94 into aiidateam:main Oct 13, 2022
@sphuber sphuber deleted the fix/5693/serialize-none branch October 13, 2022 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WithSerialize.serialize calls the serializer even if the provided value is None
2 participants