Add option to delete builder attributes

[ramirezfranciscof]

Fixes #4381

The builder object was able to delete already set inputs as
items (del builder['input_name']), but not as attributes
(del builder.input_name). This is not consistent with how
these inputs can be set or accessed (both builder.input_name
and builder['input_name'] are possible).

Also added tests for these two ways of accessing inputs.

[codecov]

Codecov Report

Merging #4419 into develop will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4419      +/-   ##
===========================================
+ Coverage    79.22%   79.23%   +0.01%     
===========================================
  Files          475      475              
  Lines        34832    34834       +2     
===========================================
+ Hits         27593    27596       +3     
+ Misses        7239     7238       -1     

Flag	Coverage Δ
#django	73.08% <100.00%> (+0.01%)	⬆️
#sqlalchemy	72.30% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/engine/processes/builder.py	92.14% <100.00%> (+1.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e1bdf2...f8a9b7d. Read the comment docs.

[giovannipizzi]

I wouldn't disable this.

[sphuber]

I agree. I have to say the cell-var-from-loop is also probably not a good idea to ignore. I know it was already there, but this might point at a bug

[greschd]

When enabling the cell-var-from-loop check, it complains about lines 52 (port) and 60 (name).

https://github.com/aiidateam/aiida-core/blob/9244969fe53b16a89309624b32a50b0c6b3ee768/aiida/engine/processes/builder.py#L41-L65

To me, the first looks like a false positive, while the second could be a bug. Here's some roughly-equivalent test code:

In [4]: funcs = []

In [5]: mapping = dict()

In [6]: for x in range(10):
   ...:     def foo(val=x):
   ...:         mapping[x] = val
   ...:         return val
   ...:     funcs.append(foo)
   ...:
   ...:

In [7]: [f() for f in funcs]
Out[7]: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

In [8]: mapping
Out[8]: {9: 9}

What's puzzling to me is that this looks like quite often-used code. Maybe both are actually false positives. But it certainly warrants looking into; cell-var-from-loop is quite a useful check in my experience, since it covers easily-overlooked corner cases of the language.

[ramirezfranciscof]

1. Re cell-var-from-loop, I'm not sure I fully understand the problem (it looks somewhat simililar to the recursively expanded and simply expanded variables that were such a headache when coding makefiles).

2. Re line-too-long, I've managed most of them but there are two that are still there and I don't know how to fix without introducing intermediate variables (all "aesthetical" modifications are reverted by yapf):

    def __dir__(self):
        return sorted(set(self._valid_fields + [key for key, _ in self.__dict__.items() if key.startswith('_')]))
(...)
    if not (isinstance(pruned, collections.abc.Mapping) and not pruned and not isinstance(pruned, Node)):
        result[key] = pruned

[greschd]

@sphuber I'm seriously confused by that code. Specifically, the setattr(self.__class__, name, getter) sets the property on the ProcessBuilderNamespace class - but doesn't that mean all ProcessBuilderNamespace instances will share the same properties?

Also, when trying to reproduce the class behavior, I can't get the fsetter to work - instead, the __setattr__ is needed. But why even have the fsetter, then?

The following code might illustrate my confusion somewhat:

class Foo:
    def __init__(self, num_vals, obj_name):
        self._data = {}
        for x in range(num_vals):
            name = chr(97 + x)

            def fgetter(self, name=name, obj_name=obj_name):
                print(f'fgetter for {obj_name}')
                return self._data.get(name)

            def fsetter(self, value, obj_name=obj_name):
                print(f'fsetter for {obj_name}')
                self._data[name] = value

            getter = property(fgetter)
            getter.setter(fsetter)

            setattr(self.__class__, name, getter)

    def __setattr__(self, attr, value):
        if attr.startswith('_'):
            object.__setattr__(self, attr, value)
        else:
            self._data[attr] = value


f = Foo(3, 'f')
print(f.a)
f.a = 2
print(f.a)
try:
    print(f.j)
except AttributeError as exc:
    print(str(exc))

g = Foo(10, 'g')
print(g.j)
print(f.j)

Output:

fgetter for f
None
fgetter for f
2
'Foo' object has no attribute 'j'
fgetter for g
None
fgetter for g
None

[greschd]

The "true positive" in our code is this line:

AH, because it is interpreting name as the external variable instead of name the internal function input parameter?

Right, because that function doesn't have a name parameter, it's just "captured" from the enclosing scope.

Do you mean, in my example you would have to pass x as the default of an "unused/artificial/forced" function parameter in order to do my list of summing functions?

funcs = []
for x in range(100):
    def foo(n, int_x=x):
         return n + int_x
    funcs.append(foo)

Mmmm It feels kind of hacky (the summing term is an internal parameter, I don't want to have to define an input that the user could potentially overwrite).

Yep, that'd work. If you want to avoid exposing the extra keyword argument but still bind it to the object, you can use functools.partial, like so:

In [1]: from functools import partial

In [4]: funcs = []
   ...: for x in range(10):
   ...:     def foo(y):
   ...:         return y
   ...:     funcs.append(partial(foo, y=x))
   ...:

Note also how you wouldn't even have to re-define foo in every loop iteration here:

In [6]: funcs = []
   ...: def foo(y):
   ...:     return y
   ...: for x in range(10):
   ...:     funcs.append(partial(foo, y=x))
   ...:

For the general behavior, there's also an entry in the Python FAQ: https://docs.python.org/3/faq/programming.html#why-do-lambdas-defined-in-a-loop-with-different-values-all-return-the-same-result

[greschd]

And here's the somewhat terse description of name binding, which is the background for why it's behaving that way: https://docs.python.org/3.8/reference/executionmodel.html#naming-and-binding

Short summary: In Python we don't really think of variables as having a certain value (like e.g. in C, C++ etc.) in the sense that that value is stored at the variable location. Instead, a "variable" is a name that binds to an object (which exists separately from the name).
Assignment, and passing arguments to functions, are "name binding" events. This also underlies the common question if arguments are passed by value or by reference (neither, they are passed by name binding!).

[ramirezfranciscof]

Right, because that function doesn't have a name parameter, it's just "captured" from the enclosing scope.

I see. I was mixing up fsetter with fgetter, but you are right, the error comes from the one with no name parameter. Ok, I think the solution is then to copy the def just above and add the name=name thing, right? And add the ignore in the specific line for that other function.

Thanks for the thorough explanation, btw! Crystal clear!

[greschd]

Ok, I think the solution is then to copy the def just above and add the name=name thing, right?

In principle yes, but I think the rest of that __init__ code doesn't do exactly what it's supposed to - which is why I opened #4420. I'm not 100% sure, but I think the fsetter is never actually invoked.

But I think it would be ok to fix it in this way here (for the false positive, just explicitly ignore the pylint error on that line). Resolving #4420 is probably not closely related to this PR - I just happened to stumble upon it while looking at this.

Thanks for the thorough explanation, btw! Crystal clear!

Happy to help!

[ramirezfranciscof]

Yeah, I just did the modifications. This way you will also be able to get "cleaner" base for the test of #4420 .

[giovannipizzi]

All good thanks! Only, I wouldn't disable the long lines check, I think that's one we want to keep. Otherwise it's good to go for me

[sphuber]

Thanks @ramirezfranciscof some requests

[sphuber]

I agree. I have to say the cell-var-from-loop is also probably not a good idea to ignore. I know it was already there, but this might point at a bug

[sphuber]

Thanks @ramirezfranciscof . Almost good to go, just revert the formatting changes and add a few comments as indicated in my comments

[sphuber]

Did a linter adjust the length of all docstrings or did you do this manually? Anyway, please revert. We are maintaining a 120 character limit for the entire project, including docstrings. Doesn't make sense to start changing this here.

[ramirezfranciscof]

Ok, maybe something in my pylint got messed up, but it didn't let me commit with lines longer than a 100 chars (that is why I originally put the # pylint: disable=line-too-long that I was requested to remove).

EDIT: to include pylint complaint

hookid: pylint

************* Module aiida.engine.processes.builder
aiida/engine/processes/builder.py:44:0: C0301: Line too long (109/100) (line-too-long)

-------------------------------------------------------------------
Your code has been rated at 9.91/10 (previous run: 10.00/10, -0.09)

[sphuber]

If there is a problem with your dev installation, it would be more sensible to try and fix that, instead of adapting the code to the broken environment, wouldn't it? Anyway, you should simply update your installation by running pip install -e .[pre-commit] which will install the correct version of pylint. The configuration has moved to pyproject.toml instead of the .pylintrc file, but this is only supported starting from pylint==2.6,

[sphuber]

Seems this docstring got reverted incorrectly

[ramirezfranciscof]

Thanks for the catch! I think it should be ready now.