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

Allow the usage of lambdas for InputPort default values #3465

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Oct 25, 2019

Fixes #1604

This is made possible by upgrading plumpy to version 0.14.3. This
functionality is useful to define defaults that should change at each
invocation, for example the generation of a seed. However, it also
provides a way to avoid the issues that arise when defining a default to
be an instance of a Node. Since a node instance is always regarded as
a mutable type by python, regardless whether it is stored or not, it can
cause unexpected behavior. For example when the node appointed to the
default value of a port is deleted in the database, the memory instance
remains and points to a no longer existing database row. The next time
it is accessed an exception will be thrown. Using a lambda avoids this
problem.

Edit: this will also address #2515 and #3143 indirectly. It will still require users to replace mutable defaults with lambdas. Therefore I prefer to close them in a separate PR where the InputPort constructor implementation will warn when mutable values are specified for the port's default

@sphuber sphuber requested a review from ltalirz October 25, 2019 11:53
@chrisjsewell
Copy link
Member

FYI this should also fix #2515 and #3143 :)

ltalirz
ltalirz previously approved these changes Oct 25, 2019
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sphuber for this fix!

a few minor comments/questions.

Also, consider to at least reference the two other related issues that we saw yesterday (about resetting the DB during tests).

aiida/backends/tests/engine/test_ports.py Outdated Show resolved Hide resolved
docs/source/working/processes.rst Outdated Show resolved Hide resolved
docs/source/working/processes.rst Outdated Show resolved Hide resolved
docs/source/working/processes.rst Outdated Show resolved Hide resolved
docs/source/working/processes.rst Outdated Show resolved Hide resolved
docs/source/working/processes.rst Outdated Show resolved Hide resolved
@ltalirz
Copy link
Member

ltalirz commented Oct 25, 2019

Ah, I see @chrisjsewell has me beat by 1 minute ;-)

@sphuber
Copy link
Contributor Author

sphuber commented Oct 25, 2019

FYI this should also fix #2515 and #3143 :)

Indirectly it would yes, but I was thinking of maybe adding a check in the InputPort constructor to warn the user when a mutable value is specified for a default. With that PR I would close those issues

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sphuber !

This is made possible by upgrading `plumpy` to version `0.14.3`. This
functionality is useful to define defaults that should change at each
invocation, for example the generation of a seed. However, it also
provides a way to avoid the issues that arise when defining a default to
be an instance of a `Node`. Since a node instance is always regarded as
a mutable type by python, regardless whether it is stored or not, it can
cause unexpected behavior. For example when the node appointed to the
default value of a port is deleted in the database, the memory instance
remains and points to a no longer existing database row. The next time
it is accessed an exception will be thrown. Using a lambda avoids this
problem.
@sphuber sphuber force-pushed the fix_1604_allow_lambda_port_defaults branch from 9574b41 to 30a4a3f Compare October 25, 2019 12:57
@sphuber sphuber merged commit 6b62fa3 into aiidateam:develop Oct 25, 2019
@sphuber sphuber deleted the fix_1604_allow_lambda_port_defaults branch October 25, 2019 13:24
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.

Allow function as default values for WorkChain input.
3 participants