Emit a warning when a node instance is set as input port default

[sphuber]

Fixes #2515 and fixes #3143

Using mutable objects as defaults for InputPorts can lead to
unexpected result and should be discouraged. We override the InputPort
constructor to check the type of the default, if specified and emit a
warning if it is not a lambda, i.e. a callable or an immutable type. The
check for immutability is not well-defined in python so we check whether
the default has a type within a set of known immutable types. This means
that there can be obscure immutable types that trigger a false positive
but since all we do is print a UserWarning this is acceptable.

The CalcJob implementation had to be adapted to conform with the new
requirements as the mpirun_extra_params and environment_variables
metadata options were using mutable types for their default. The
defaults have been changed to the list and dict base types which
when not instantiated act like callables and therefore respect the
rules.

[ltalirz]

Thanks @sphuber ! Some minor suggestions on the PR itself.

Apart from this, I think before deciding whether to merge this we should try to collect input from a few plugin developers as well, since I suspect this change is going to affect a large number of people - and not just plugin developers, but also people who just write workflows.

Before this change, we are in a space where you can choose to use lambdas, but you can also choose not to.

In this change, we are saying: lambdas are the way to handle default values for ports with AiiDA data types (not yet enforced, but you'll get tedious warnings otherwise).

Are we sure this is the best solution going forward? If yes, fine to merge. If not, perhaps better to wait a bit?

I would be glad to get some feedback on this; e.g. from @giovannipizzi @greschd @dev-zero @chrisjsewell

[ltalirz]

Could you perhaps provide a reference where this list comes from?

[dev-zero]

... and: typo, should be immutable_types

[dev-zero]

But I agree with @ltalirz: needs documentation, and I maybe implement it as a static function from the start since a candidate from Python 3.7 would be @dataclasses.dataclass(frozen=True) decorated class?
There you don't get a type you could check against but would have to do a separate check (if isdataclass(...) and hasattr(..., '__setattr__')).

[sphuber]

I couldn't find any definitive resource in the official python docs. I agree that this is fragile.

[ltalirz]

Suggested change

	'mutable default defined for port `{}` which is highly unadvisable. Please use an immutable '
	'mutable default for port `{}` which can lead to unexpected side effects. Please use an immutable '

[ltalirz]

Suggested change

	'value instead or employ a lambda.'.format(name)
	'value instead or employ a lambda function - e.g. replacing `Int(5)` with `lambda: Int(5)`.'.format(name)

This suggestion might not be the final solution but I think if we want to force everybody to do this in their WorkChains we should try to give them a bit more of a hand.

[sphuber]

Also CalcJobs. If you think this will help, we can put it in.

[ltalirz]

I get what this does and that this is now no longer a "mutable" argument, but I believe this will be difficult to understand to new users.

[sphuber]

What is there to understand though? They won't look in the implementation I imagine and with the ProcessBuilder auto help it will show default=list, which seems fine no? Anyway, I wouldn't know how to solve this otherwise, because putting None will break backwards-compatibility.

[ramirezfranciscof]

This might sound silly, but maybe we could use default=list() and default=dict()? At least for me, the parentheses (is that the plural?) make it clearer that you are creating an empty list/dict and not passing the actual class.

[sphuber]

the problem is though, that as soon as you add the parentheses you are calling the callable and so you are instantiating it. That is exactly what we don't want to do. The builint list is a callable and calling it like list() creates an instance of that type. This is just the way python works.

[greschd]

I think part of why this might be hard to catch for new users is naming: The default argument essentially combines the concepts of a default and a default_factory (in the sense of defaultdict or dataclasses.field).

In my opinion the interface would be cleaner if the two concepts were separated, but for backwards compatibility I would only change this very slowly, if at all.

[dev-zero]

in general I fail to see how this fully solves #2515, allowing lambdas (or arbitrary callables for that matter) allow for any possible side-effect. What am I missing?

[sphuber]

in general I fail to see how this fully solves #2515, allowing lambdas (or arbitrary callables for that matter) allow for any possible side-effect. What am I missing?

It "solves" it as in it will instruct people not to do things that definitely lead to problems. I agree that it doesn't actually solve the underlying problem, but I am not sure that that is even possible. A similar problem can occur when a node is created in interpreter A, is loaded in B and then deleted. If A now accesses any attribute, it will also except. What should the behavior in this case be? Would it even be possible to catch this exception and handle in such a way that always provides a well defined solution? At least I don't know of one.

[sphuber]

Anyway, given that the effects of this change are quite uncertain, we should probably not include this now and push this until after the release.

[giovannipizzi]

Ok to postpone emitting the warning if we are unsure (even if I don't see a real use case where this is a problem, but ok to double check with all developers first) - still, I would merge the replace of all other defaults with lambdas, because I think it's the right thing to do to avoid to essentially share the mutable among all classes. I post also this example for those who are not aware of this:

def test_mutable(a=[]):
    print(id(a), a)
    a.append('1')
    print(id(a), a)

if __name__ == "__main__":
    test_mutable()
    test_mutable()

Result:

(4424249640, [])
(4424249640, ['1'])
(4424249640, ['1'])
(4424249640, ['1', '1'])

I.e., the default is instantiated once and reused between different calls.

[chrisjsewell]

Well it certainly makes sense to me. Mutable default arguments, in python, are a definite no no, and using a default_factory is exactly what is now implemented in python 3.7's dataclasses.field

[greschd]

To be honest, I wouldn't have realized setting something like orm.Bool(True) as default is an issue - maybe because it's associated with the Python bool, which is immutable.

On the other hand, isn't an ORM data class supposed to be immutable (up to extras) once it's stored?

In the spec.input, we basically have control over what to do with the default before handing it off to the user - so we could also decide to .store() ORM classes there, and return a clone (or copy.deepcopy for non-ORM classes) when the process is instantiated. I'm not sure if that's a better behaviour though, there might be other issues with such an approach.

Regarding the list of "immutable" types, it's not even always clear from the type if something is safe. For example, types.MappingProxyType gives you a read-only proxy to a mapping. This means types.MappingProxyType({}) is a good default for "empty dict" (because the dict it wraps is pretty well hidden away), but if the mapping it's wrapping is accessible somewhere else it would still be mutable in that way.

[dev-zero]

btw, all the builtin types are callables, so this can be simplified to:

             if default is not ports.UNSPECIFIED and not callable(default): 

[chrisjsewell]

so we could also decide to .store() ORM classes there, and return a clone

Yeh definitely don't do that lol. Remember the Process.define is a classmethod, so it will only be called once per python session/kernel (for a particular Process sub-class). You then end up, arbitrarily, with a single default node (irrespective of whether you return a clone) incoming to all Processes initiated in the same session, and a different one for the next session, etc. I wouldn't say that should be the expected behavior and, as already noted in #3143, this can already break test sessions where a Process is used in more than one test.

[greschd]

Well, the "clone" part should happen when the process is instantiated (not sure if this is even possible, though), so this would still give you separate nodes.

But you're right, I hadn't read the issues leading up to this well enough - it wouldn't fix the issue of relying on nodes that would disappear after a DB wipe.

[ramirezfranciscof]

Anyway, given that the effects of this change are quite uncertain, we should probably not include this now and push this until after the release.

@sphuber what is the status of this? I'm assuming the release mentioned was 1.0, which has already passed, correct? Is this something we should rebase and try to merge / further discuss in some meeting / discard?

[sphuber]

I had put it on hold because effectively we could not come to an agreement on what the behavior should be. To summarize: using mutable types as function defaults in python is not encouraged because it can have unexpected side-effects. For some similar (with subtle differences) reasons, it is better to also not use AiiDA nodes as defaults in process specification definitions, i.e.:

class SomeProcess(Process):

    @classmethod
    def define(cls, spec):
        super().define(spec)
        spec.input('x', default=Int(5))

The reason is that with this approach the node Int(5) is created as soon as the file is imported. The first instantiation of that process with that default will store the node and all subsequent launches will receive the exact same node. The same goes for wrapping processes that expose the spec of this base process. The reference to the default will be copied and used everywhere else. This can have unexpected results when the node is deleted, but than referenced later. For example in a test environment, if you launch the process, Int(5) gets stored. If you then delete the process node at the beginning of the next test, so does the Int(5) node. However, the reference in memory still exists in the process spec. If you now execute the process again, we will get an exception as the node in memory will try to access the node in the database that no longer exists.

All of this can be easily prevented by making the default a callable, for example with a lambda. The change is minimal:

class SomeProcess(Process):

    @classmethod
    def define(cls, spec):
        super().define(spec)
        spec.input('x', default=lambda: Int(5))

The default value is just prefixed with lambda:. This PR simply proposes to print a warning if a default is specified that is mutable.

• Advantage: warns people if they do something that may have unexpected problematic consequences
• Disadvantage: requires people to prepend lambda: for mutable defaults if they want to suppress the warning

I propose we vote here, please put your name where applicable:

• Warning for all mutable types: @sphuber @ramirezfranciscof
• Warning for AiiDA nodes only: @greschd (@sphuber) @ltalirz
• No warnings:

[greschd]

A middle ground would be warning only on the AiiDA node defaults - I think this part is less controversial, and we don't have to go into how to determine if a general Python object is mutable.

If you won't allow that option in the vote, I'd go with "in favor".

[ltalirz]

Given the problems people are facing with compiling the documentation on RTD, which mostly stem from this issue, I agree that issuing a warning would be practically useful at this point (even if I still find the need to pass lambdas not very elegant... not that I have a better solution either, though).

[ramirezfranciscof]

(1) @sphuber : I see the problem clearly with the aiida nodes, but why is this discouraged for all mutable types? (I'm thinking of lists vs tuples here, correct?)

(2) @ltalirz @greschd The lambda thing seems to be kind of a "hack" to go around the warning, but if I'm not mistaken what we are ultimately trying to encourage here is the use of immutable types, so technically the way to really ("officially"?) solve the warning is to change to an immutable type for the default. Can you think of an example where this would not be possible?

[sphuber]

(1) @sphuber : I see the problem clearly with the aiida nodes, but why is this discouraged for all mutable types? (I'm thinking of lists vs tuples here, correct?)
A tuple is immutable in python. But yes, I am referring here to mutable builtins like lists and dictionaries. Consider the following example using a mutable type as default:

In [12]: def print_something(l=[]):
    ...:     l.append('a')
    ...:     print(l)
    ...:     

In [13]: print_something()
['a']

In [14]: print_something()
['a', 'a']

Executing the function twice consecutively will give a different behavior. This is what is meant with "unexpected behavior" when using mutable types for function defaults.

(2) @ltalirz @greschd The lambda thing seems to be kind of a "hack" to go around the warning, but if I'm not mistaken what we are ultimately trying to encourage here is the use of immutable types, so technically the way to really ("officially"?) solve the warning is to change to an immutable type for the default. Can you think of an example where this would not be possible?

I wouldn't call it a hack. The lambda prefix simply makes the default a callable which is perfectly valid for defaults

[greschd]

The important bit of the lambda solution is that the "function body" is not executed when the class is defined, but only when the process is constructed. This solves the problem in two ways:

• Just creating the spec does not need the backend, which was the source for the documentation troubles
• Each time a new process is created a new node is created, resolving the issue of depending on nodes which might disappear (in tests), and generally making the graph more consistent.

[ramirezfranciscof]

(1) @sphuber : I see the problem clearly with the aiida nodes, but why is this discouraged for all mutable types? (I'm thinking of lists vs tuples here, correct?)

A tuple is immutable in python. But yes, I am referring here to mutable builtins like lists and dictionaries.

Yes, yes, that's what I meant with tuples vs list as my go-to example for immutable vs mutable.

Consider the following example using a mutable type as default:

In [12]: def print_something(l=[]):
    ...:     l.append('a')
    ...:     print(l)
    ...:     

In [13]: print_something()
['a']

In [14]: print_something()
['a', 'a']

Woah, ok, I would not have expected that behavior. I'm not sure in what kind of situations this behavior would be the desired one (it looks like the save attribute in FORTRAN). What is the criteria for a non-intrinsic type to be considered mutable or immutable? I would think almost all would end up being mutables, right? I'm leaning towards warning for all mutables (actually, I'm surprised pylint doesn't already do this).

[greschd]

I'm surprised pylint doesn't already do this

Pylint does do this, but only when it's actually in a function definition. Here it's passed as a "regular" parameter to the spec.input function, which pylint can not recognize as being semantically the same.

[greschd]

There are some cases where you actually want that behavior (to keep state between function calls).

But more than that, it's a (slightly inconvenient) consequence of "everything is an object", and the way function arguments are passed in Python (by binding a new name to an existing object).

I don't want to take this thread entirely off the rails, but just to clarify this: Python does not distinguish between immutable or mutable objects when calling a function, they are treated the same. The difference is that immutable objects have no mutating methods.

It's almost impossible to make a custom class immutable, but there are things like frozendict (https://pypi.org/project/frozendict/) where at least the public API does not have mutating methods.. so if you mutate it, you'd better know what you're doing. Using a frozendict default argument is fine, really.

And on the other hand, even for built-ins it's hard to check if something is completely immutable. A tuple can contain immutable elements, for example:

In [12]: def print_something(l=tuple([[]])):
    ...:     l[0].append('a')
    ...:     print(l)
    ...:     

In [13]: print_something()
(['a'],)

In [14]: print_something()
(['a', 'a'],)

[ramirezfranciscof]

I propose we vote here, please put your name where applicable:

Warning for all mutable types: @sphuber @ramirezfranciscof
Warning for AiiDA nodes only: @greschd (@sphuber) @ltalirz
No warnings:

@sphuber do you want to ping someone else who (should vote for / may have an opinion on) this?

[sphuber]

I guess the other persons with a strong opinion last time around were @chrisjsewell and @dev-zero with the former being in favor of the warning and the latter against. If @dev-zero is ok with a warning for AiiDA nodes, then I think we can move ahead with that one. That seems like the best and safest option right now

[sphuber]

@greschd @ltalirz I have reduced the scope of the warning to only node instances and rebased the PR. The comment by Leopold about the new defaults for the environment_variables and mpi_extra_parameters has also been addressed. I simply made them not required and deal with the None value in the presubmit.

[chrisjsewell]

with the former being in favor of the warning

Yep the warning only for nodes sounds good 👍 , as all mutable type would just be too hard to assess.

[greschd]

All good, just a small comment on one of the tests.

[greschd]

Side note: If this is a common idiom, we might want to add a default keyword argument to get_option also.

[sphuber]

yeah, the get_attribute that it calls through to already has this. Don't want to change that in this PR though

[ltalirz]

thanks @sphuber - just one question

[ltalirz]

I'm wondering a bit - doesn't it make sense to have the default value specified in the port itself rather than outside?

In the mode above, anyone using the port has to "know" what the default value is.

I.e. I guess in the current port syntax, default=lambda: [] is perhaps the way to go...

[sphuber]

I realized another disadvantage of specifying an empty list or dictionary as default, by doing that, each CalcJobNode will always have those attributes, even though they are almost always empty.

{
    "environment_variables": {},
    "mpirun_extra_params": [],
}

By not specifying "empty" defaults, we prevent these unnecessary attributes. The same actually goes for all other options with "empty" defaults, like prepend_text and append_text. Thinking about it now, maybe I should not change this behavior in this PR but we should think about this whether this is desirable behavior. I will implement the change you proposed and then open an issue

[greschd]

All good for me, but let's wait for @ltalirz to check if his comment was resolved.

[ltalirz]

all good to go, thanks guys!