Fix autocompletion for LinkManager and AttributeManager

[giovannipizzi]

In the managers, exceptions were not caught properly.
For instance, getattr(calc.inputs, 'value') was not return
an AttributeError if 'value' does not exist, and as a
consequence getattr(calc.inputs, 'value', None) was raising
instead of returning None.

Similarly, I fixed calc.inputs['value'] to raise a KeyError
for not-existing value.

The AttributeManager was not having Tab completion for a different
reason, __dir__ was returning tuples of dict items, rather than
just the keys.

Now these are fixed and tests are added (I cannot really test
the TAB-completion functionality, but at least I test that the
values of dir() and the exceptions raised are the correct ones).

Fixes #3984

[giovannipizzi]

I'm realising now that plugin developers might have made assumptions on the exception returned.
However, I'm convinced that it's not a good idea not to raise something different than a AttributeError because it's something unexpected and e.g. tab completion will not work.

I don't know if this is something we accept and we alert all plugin developers that in 1.3 this behaviour is changing, or there is something else we can do that is more backward-compatible.
But I would consider this a bug, and not wait for a 2.0 to be out to re-enable TAB-completion.

One option would be to create for now a combined class NotExistentAttributeError that inherits both from AttributeError and NotExistent? Would this be working and tenable also in the future when releasing a 2.0?

Pinging @sphuber @ltalirz for feedback

[codecov]

Codecov Report

Merging #3985 into develop will increase coverage by 0.06%.
The diff coverage is 88.89%.

@@             Coverage Diff             @@
##           develop    #3985      +/-   ##
===========================================
+ Coverage    78.45%   78.50%   +0.06%     
===========================================
  Files          461      461              
  Lines        34094    34077      -17     
===========================================
+ Hits         26744    26750       +6     
+ Misses        7350     7327      -23     

Flag	Coverage Δ
#django	70.55% <88.89%> (+0.06%)	⬆️
#sqlalchemy	71.40% <88.89%> (+0.06%)	⬆️

Impacted Files	Coverage Δ
aiida/parsers/plugins/arithmetic/add.py	82.15% <66.67%> (-0.61%)	⬇️
aiida/orm/utils/managers.py	91.81% <100.00%> (+31.81%)	⬆️
aiida/transports/plugins/ssh.py	76.30% <0.00%> (-0.08%)	⬇️
aiida/orm/nodes/data/cif.py	87.13% <0.00%> (+0.55%)	⬆️

---

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 6220e85...ee01669. Read the comment docs.

[greschd]

I totally agree that raising NotExistent on __getattr__ and __getitem__ is not great - I've also encountered the getattr(obj, 'key', default) idiom not working before.

However, I'm also convinced that we need a backwards-compatible solution for this. The NotExistent exception is caught in many places. Across the plugins that I happen to have cloned on my machine, there are 23 instances of except NotExistent:

$ grep -inR 'except NotExistent' . | grep -v core | wc -l
23

I'd favor adding a NotExistentAttributeError and NotExistentKeyError, but it would also be nice to have a way to eventually deprecate this. Not sure how we could know if the user code catches it as AttributeError or NotExistent, though.

[giovannipizzi]

Not sure how we could know if the user code catches it as AttributeError or NotExistent, though.

Indeed, this is my main concern... It would be great if there was a clever way to check if they are catching NotExistent to warn them.
But maybe we just make it backwards-compatible, and inform all plugin developers, and adapt the documentation...

[greschd]

Yeah, it's a difficult one.. if we were to phase out the NotExistent exception completely, we could warn on import. But I'm guessing there still is a valid use case for this exception, right?

That also means there'd still be a valid use-case for catching NotExistent, just not when it's triggered by either __getattr__ or __getitem__. That's pretty much impossible to create a deprecation warning for.

[giovannipizzi]

Yeah. And actually, thinking better to it, indeed, we don't really need to deprecate the old behaviour.
I think we can keep all three catching behaviours acceptable: AttributeError, NotExistent and NotExistentAttributeError.

I just pushed the backwards-compatible fix to the code, and I think it's now OK (I tested and tab completion works) and shouldn't break backwards-compatibility

[sphuber]

Yeah, it's a difficult one.. if we were to phase out the NotExistent exception completely, we could warn on import. But I'm guessing there still is a valid use case for this exception, right?

Absolutely. It may have been misused in the managers, but I don't think we should get rid off it entirely. It is used in various methods where it is actually just fine. For example in the QueryBuilder.one will return NotExistent, which makes perfect sense and more so than an AttributeError or KeyError.

Couldn't we also make NotExistent a subclass of AttributeError. That way, we can change the managers to throw AttributeError and existing code will continue to work. But not sure if this has undesirable effects elsewhere

[giovannipizzi]

Absolutely. It may have been misused in the managers, but I don't think we should get rid off it entirely. It is used in various methods where it is actually just fine. For example in the QueryBuilder.one will return NotExistent, which makes perfect sense and more so than an AttributeError or KeyError.

Sure, the plan was not to drop the NotExistent exception.

Couldn't we also make NotExistent a subclass of AttributeError. That way, we can change the managers to throw AttributeError and existing code will continue to work. But not sure if this has undesirable effects elsewhere

I think the current approach is better. It's backwards-compatible, and we don't risk that people catch a AttributeError and end up catching a NotExistent, instead. Also, we would need to subclass NotExistent also both from AttributeError and KeyError, that I don't think it's a great idea. For me the current code in the PR is a good final solution.

[greschd]

I think the problem with subclassing NotExistent from AttributeError is that it would also need to subclass from KeyError - because it can be the cause of either one of them.

I'm fine with the current solution, but would move the NotExistentAttributeError and NotExistentKeyError to be internal - i.e., remove it from the __all__ and probably add a leading underscore. If people start catching these explicitly, we will be in even more of a backwards-compatibility mess.
I think, eventually, we might get to a point where we can drop these and just raise the bare AttributeError and KeyError.

[giovannipizzi]

Ok, let's wait for another opinion before I change the code.
I was thinking that in the end all the exceptions are OK, even to be kept as public: they are specific extensions of very well defined exceptions (python's or AiiDA's) and, now that I think to I, I think it's ok if people catch any of these.

It's a bit like now in Python3 you have a FileNotFoundError that actually is a subclass both of IOError and OSError, while in python2 you would e.g. get IOError if calling open on a non-existing file.

And actually it's ok if you catch the more specific exception (if you want to). No?

[greschd]

Hmm, I think you're right - but it should definitely be carefully considered before we commit to this. As we've seen, exception types are kind of hard to change in a backwards-compatible way.

[sphuber]

Thanks @giovannipizzi I am just wondering if we should still think about some deprecation path for the old NotExistent raising behavior?

[sphuber]

Thanks @giovannipizzi !