Fix wrong raise from dict nodes

[ramirezfranciscof]

When accessing an attribute as a dict (dictnode['key']), the error raised was an AttributeError instead of a KeyError, which is rather unusual. This has been fixed and a test that checks the correct error raise has been added.

Fixes #4454

[codecov]

Codecov Report

Merging #4577 (a5717e8) into release/1.5.1 (8058a97) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@                Coverage Diff                @@
##           release/1.5.1    #4577      +/-   ##
=================================================
+ Coverage          79.50%   79.51%   +0.01%     
=================================================
  Files                482      482              
  Lines              35320    35323       +3     
=================================================
+ Hits               28078    28082       +4     
+ Misses              7242     7241       -1     

Flag	Coverage Δ
django	73.68% <100.00%> (+0.01%)	⬆️
sqlalchemy	72.84% <100.00%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
aiida/orm/nodes/data/dict.py	86.05% <100.00%> (+1.05%)	⬆️
aiida/engine/daemon/runner.py	82.76% <0.00%> (+3.45%)	⬆️

---

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 8058a97...a5717e8. Read the comment docs.

[sphuber]

Changes look fine to me, I am just wondering about backwards compatibility. Some code out there may rely on this (maybe incorrectly) raising an AttributeError instead of a KeyError. Thoughts @ltalirz and @giovannipizzi ?

[ramirezfranciscof]

Also paging @yakutovicha who run into this problem originally and actually had to code around it.

[ltalirz]

Hm... that's a tricky one.
There are quite a lot of search results on github - not all of them would break but some of them would, e.g.
https://github.com/electronic-structure/aiida-exciting/blob/c7f4d2d2370dc0b5cb1270883ff24d4880a11dad/aiida_exciting/calculations/exciting.py#L148-L153

In that particular case it wouldn't be too bad since an exception is raised anyhow but it's possible that other plugins catch the exception and do something else.
In principle this is a breaking change.

I don't have a strong opinion on whether to wait until 2.0 with this or not.

[giovannipizzi]

I fear here we have to define a mixed exception that inherits from both?
The question, however, is how to issue a deprecation warning if people catch the AttributeError?
Maybe we just mention in the docs and will remove in 2.0 and for now we just put in the list of things to mention for backward-incompatibility (and we already inform plugin developers to catch the new one, or even both, from the next 1.x release).

Or we accept that forever in the future the exception raised there is a mixed exception (BTW, the aiida-exciting plugin is still at version 0.x so I wouldn't worry about it, but it's true that others might be catching it.

[yakutovicha]

Changes look fine to me, I am just wondering about backwards compatibility. Some code out there may rely on this (maybe incorrectly) raising an AttributeError instead of a KeyError. Thoughts @ltalirz and @giovannipizzi ?

I think people, who relied on the "incorrect" behaviour should have put a catch for both: AttributeError and KeyErrorexceptions, otherwise, they would experience the same issue as I: aiidateam/aiida-cp2k#116

[ramirezfranciscof]

This behaviour was introduced in version 1.4.0 and is present in all 1.4.x and 1.5.0 versions, how do you want to proceed @sphuber ?

[sphuber]

Not sure, v1.4 has been out for over 2 months and since this bug was introduced there, we should technically fix it in v1.4 and release v1.4.4.

[ramirezfranciscof]

I mean, I think this fix should be simple enough that I can do a fix in 1.4.4 and 1.5.1 (you can tell me exactly how best to proceed to do this). But the question is do we need to concern ourselves with backwards compatibility for this?

[sphuber]

I mean, I think this fix should be simple enough that I can do a fix in 1.4.4 and 1.5.1 (you can tell me exactly how best to proceed to do this).

Just create a support/1.4.x that branches of v1.4.3 and change this PR to merge on to that support branch. Since master is already at v1.5 we are not going to merge this support branch in, but we release directly off of that branch. Then you simply create another PR onto release/1.5.1 (which I already created), cherry-picking then this merged commit that fixes the problem. Once we release v1.5.0 we also merge that back into develop which will then also have the fix.

But the question is do we need to concern ourselves with backwards compatibility for this?

No. Since we simply added a bug in v1.4 and we are now fixing it in a patch version of the minor branch, backwards compatibility doesn't really apply here.

[sphuber]

@ramirezfranciscof can you rebase this on release/1.5.1 and update the PR so we can merge this? It would be good to release 1.5.1 a.s.a.p.

[ramirezfranciscof]

@sphuber hey, I'm not sure what is happening here. The two tests for 3.6 are failing during installation and django 3.8 (but not sqlalchemy 3.8?) is failing due to an error that doesn't seem to be related to dicts.

[sphuber]

See this issue. I have already opened PR #4615 to fix it. Just needs to be approved. That explains the failures under Python 3.6. The Python 3.8 Django failure is just a fluke, just look at the logs.

[ramirezfranciscof]

@sphuber this one should be ready to merge.

[sphuber]

I am getting a déjà vu...