Mutable datatypes initialized with unsafe defaults

[rafmudaf]

Mutable datatypes initialized with unsafe defaults

Some of the attrs-derived classes in floris.simulation may be initialized with unsafe defaults for mutable data types (mutable vs immutable types in Python). In Python, list, set, and dict types are all mutable, and variables referring to mutable types act like pointers to the same memory. For mutable-type attributes in attrs-derived classes with defaults of empty instances of the mutable type, any instance of the class will refer to the same memory for these particular attributes.

See #750 (comment) for the initial report.

Example

from attrs import define, field

@define
class A:
    x: list = field(default=[])
    y: list = field(factory=list)

a1 = A()
a2 = A()

print(a1, a2)
a1.x.append(4)
a1.y.append(5)
print(a1, a2)

The code above yields the following output:

A(x=[], y=[]) A(x=[], y=[])
A(x=[4], y=[5]) A(x=[4], y=[])

The first print-statement shows that the instances of class A are initialized with empty lists for both x and y attributes. In the second print-statement, appending a value to both attributes for the a1 instance is shown to also affect the x attribute of the a2 instance. The y attribute is not affected because it is declared with factory=list so a new instance of list is created for new instances of A.

This is also true of NumPy arrays, as shown below. NumPy arrays don't have an append method, but they are also mutable and their values can be changed directly leading to this same side effect.

from attrs import define, field
import numpy as np

@define
class A:
    x: np.array = field(default=np.array([1,2,3]))

a1 = A()
a2 = A()

print(a1, a2)
a1.x[0] = 9
print(a1, a2)

A(x=array([1, 2, 3])) A(x=array([1, 2, 3]))
A(x=array([9, 2, 3])) A(x=array([9, 2, 3]))

Impacts on FLORIS

The attrs library is used only in floris.simulation, so this issue is isolated to that package. While many class definitions have this issue, the use of append is generally discouraged for performance considerations. However, there are many instances where the values in NumPy arrays are modified directly including all of the solvers.

The ultimate impact is unclear, but I'll update this section as this issue is understood more.

Affected classes

Here's a first-pass list at affected classes.

• Nearly all attributes in Farm
• Nearly all attributes in FlowField
• Empirical Gauss velocity model defaults

[RHammond2]

@rafmudaf I'm adding some commentary from our discussion about how this can be resolved for default mutable objects that have initial values, such as https://github.com/NREL/floris/blob/main/floris/simulation/wake_velocity/empirical_gauss.py#L68-L69

Option 1: Use a tuple

from attrs import define, field

@define
class A:
    x: list[float] = field(default=(1.1, 2.2), converter=list)
    y: list[float] = field(default=(3.4,), convert=list)  # NOTE: in this case a comma must be added after a single element tuple

a1 = A()
a2 = A()

print(a1, a2)
> A(x=[1.1, 2.2], y=[3.4]), A(x=[1.1, 2.2], y=[3.4])

a1.x.append(3.3)
print(a1, a2)
> A(x=[1.1, 2.2, 3.3], y=[3.4]), A(x=[1.1, 2.2], y=[3.4])

Option 2: Use a list

from attrs import define, field

@define
class A:
    x: list[float] = field(default=[1.1, 2.2], converter=list)
    y: list[float] = field(default=[3.4], convert=list)

a1 = A()
a2 = A()

print(a1, a2)
> A(x=[1.1, 2.2], y=[3.4]), A(x=[1.1, 2.2], y=[3.4])

a1.x.append(3.3)
print(a1, a2)
> A(x=[1.1, 2.2, 3.3], y=[3.4]), A(x=[1.1, 2.2], y=[3.4])

The only catch to the above is if a non-numerical value is being used for an initial value, or a user inputs a single item instead of a list. To avoid the error, and allow for that case, a custom list conversion method like the one I use for WOMBAT could be used (this one is a base for partial methods, so something simpler could easily be used).

[paulf81]

hi @rafmudaf, I'd like to try to help but am still on shaky ground when it comes to attrs. But! I did some studying up tonight and hope I can be helpful. I think though I'm going to start out by saying things you already know/said to make sure I really get it:

Stuff you already know about python

1. attrs is not finally too different from normal functions and classes
2. the problem of mutable defaults in functions inputs referencing the same memory is a deep issue in python, I mean, there is no plan to "fix it" since it is the intended behavior
3. Given that, the solution/advice I've come across the most often is to never use mutables in function defaults but instead use None as the default, then assign

So for example: https://web.archive.org/web/20200221224620id_/http://effbot.org/zone/default-values.htm says, instead of:

def myfunc(value=[]):

instead write:

def myfunc(value=None):
    if value is None:
        value = []

What does this have to do with attrs

And now I think coming back to attrs, this is already accomplished via attrs.Factory:

https://stackoverflow.com/questions/44141104/when-and-why-should-i-use-attr-factory

So then I think I just propose we should use factory to resolve this issue.

Getting to the point

In your first example above you already used factory, but I think the trick is how to use factory when we want the default to be something other than an empty list, this I think can be solved with a small lambda function. So taking your original code example:

from attrs import define, field
import numpy as np

@define
class A:
    x: np.array = field(default=np.array([1,2,3]))

a1 = A()
a2 = A()

print(a1, a2)
a1.x[0] = 9
print(a1, a2)

Gives:

A(x=array([1, 2, 3])) A(x=array([1, 2, 3]))
A(x=array([9, 2, 3])) A(x=array([9, 2, 3]))

But:

from attrs import define, field
import numpy as np

@define
class A:
    x: np.array = field(factory=lambda: np.array([1,2,3]))

a1 = A()
a2 = A()

print(a1, a2)
a1.x[0] = 9
print(a1, a2)

Gives:

A(x=array([1, 2, 3])) A(x=array([1, 2, 3]))
A(x=array([9, 2, 3])) A(x=array([1, 2, 3]))

Proposal

Going to the code snippet @RHammond2 pointed out:

wake_expansion_rates: list = field(default=[0.023, 0.008])
breakpoints_D: list = field(default=[10])

Modify this to:

wake_expansion_rates: list = field(factory=lambda: [0.023, 0.008])
breakpoints_D: list = field(factory=lambda: [10])

[RHammond2]

@paulf81, I think I often forget that lambda functions are a thing, but that's potentially a much cleaner solution. Your understanding of the underlying issue is spot on that it isn't anything specific to attrs, but Python generally.

[paulf81]

Thanks @RHammond2 !

[rafmudaf]

This is an excellent summary and proposal @paulf81! An another option, how about using tuple instead of list at least in the one case for the Empirical Gauss model parameters?

wake_expansion_rates: tuple = field(factory=(0.023, 0.008))
breakpoints_D: tuple = field(factory=(10,))

I discussed this with @RHammond2 yesterday and they brought up the point that it's pretty easy to incorrectly create a single element tuple: (10) vs (10,).

[paulf81]

I think also if you use a tuple, since it's not mutable, you can also stick with default (instead of factory).

But in this case I wonder then that tuple's immutability is actually a problem. For example, wake_expansion_rates are meant to be tuned, and so changing one of the parameters in the list is not uncommon, right @misi9170 ?

So the solution is if we can't come up with a use-case for mutability, we use a tuple/default, but if we can we use list-array/factory-lambda?

[paulf81]

@rafmudaf , did you want me to propose a pull request based on this solution?

[rafmudaf]

I think also if you use a tuple, since it's not mutable, you can also stick with default (instead of factory).

This sounds good to me. Maybe we should think of a more general rule for declaring attrs-class attributes so that we don't have to remember these nuances. How about we adopt the pattern that any attribute with an empty container-class by default uses the container-classes factory?

Even though these would be safe:

field(init=False, default=tuple())
field(init=False, default="")

we would use these:

field(init=False, factory=tuple)
field(init=False, factory=str)

alongside these:

field(init=False, factory=list)
field(init=False, factory=dict)

As for the lambda, generally I think that would reduce the readability for a research software encouraging community contribution since lambdas are not common, but in this case it seems like the right technical solution. @paulf81 go for it with this fix, if you're up to it. When it's ready for review, please tag me and @RHammond2 in a pull request.

[RHammond2]

@rafmudaf, I think it makes good sense to take the factory approach as a general rule.

For the lambdas, I wonder if it just makes more sense to use the converter=list argument from my my proposal above if it allows for clarity in the default value, will still fail for inputs that can't be converted, and avoids the memory issues due to creating a new list of the default as the first step.

x: list[float] = field(default=[1.1, 2.2], converter=list)

[paulf81]

I can be persuaded either way, although i talked myself into the factory is more like the solution of if none: return [], so I thought if a little confusing still pythonic, and I can add a note. At any rate, I'll take a pass at a pull request and we can discuss more, I think a lot of this is a sort of manual search and replace so hopefully won't take me long.

Only question @rafmudaf , should. I start from develop, or some other branch?

[rafmudaf]

This will go into v3.6 so you can do it on the develop branch. Also, I made an initial list of classes affected in the original post so that should help you get started.