Node: add the copy_tree method

[sphuber]

Fixes #4928

This method allows to easily copy the entire repository tree, or a
subdirectory, of a Node to a place on the local file system. The
method simply forwards to the aiida.repository.Repository class, where
the functionality is actually implemented using the Repository.walk
method, recursively looping over all the files contained within the
node's repository.

[sphuber]

@ltalirz @chrisjsewell @ramirezfranciscof any takers?

On a side note, this PR made me think whether we need to add the possibility to create an empty directory in the virtual hierarchy of nodes. Currently this is not possible.

[codecov]

Codecov Report

Merging #5114 (4c92cec) into develop (fb259b7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5114      +/-   ##
===========================================
+ Coverage    80.83%   80.84%   +0.02%     
===========================================
  Files          534      534              
  Lines        36951    36974      +23     
===========================================
+ Hits         29865    29889      +24     
+ Misses        7086     7085       -1     

Flag	Coverage Δ
django	75.67% <100.00%> (+0.02%)	⬆️
sqlalchemy	74.77% <100.00%> (+0.02%)	⬆️

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

Impacted Files	Coverage Δ
aiida/orm/nodes/repository.py	94.19% <100.00%> (+0.14%)	⬆️
aiida/repository/repository.py	98.60% <100.00%> (+0.16%)	⬆️
aiida/transports/plugins/local.py	81.66% <0.00%> (+0.26%)	⬆️

---

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 fb259b7...4c92cec. Read the comment docs.

[ramirezfranciscof]

All looks good! I just have 3 small comments/questions before approving.

[chrisjsewell]

Eurgh, yeh it needs to be exposed in some fashion, but you are adding way too many methods on to Node, and there's absolutely no naming consistency, to identify what methods are associated with the repository (or anything else). It's just very user unfriendly 😬

This is why I've already looked into refactoring all this as e.g. Node().repo.method (#4938), which I may well circle back round to at some point

[chrisjsewell]

Basically I would point you towards: https://python-patterns.guide/gang-of-four/composition-over-inheritance/#dodge-mixins: Using "Composition over inheritance", is something we do ok in some areas of aiida, but poor in others

[sphuber]

Eurgh, yeh it needs to be exposed in some fashion, but you are adding way too many methods on to Node, and there's absolutely no naming consistency, to identify what methods are associated with the repository (or anything else). It's just very user unfriendly grimacing

This is why I've already looked into refactoring all this as e.g. Node().repo.method (#4938), which I may well circle back round to at some point

I am not against improving the interface and I see the point for exposing all repository related methods under the repo namespace. The main problem is that we cannot change this now, since we introduced this naming in v1.0 and don't want to break it for v2.0. Fine to start thinking about a better solution and introduce it at some point, deprecation the current interface.

Also very happy to have suggestions for the name of this method. I am not at all married to it. I simply took it as that is the name that is used in shutil.

Finally, I agree that too many methods at some point makes things difficult to oversee and in that respect it would be good to cleanup the Node namespace a bit. However, I don't think we will ever get to a really minimal interface. The reason is that within AiiDA the Node takes such a central role, that all reasoning, with respect to the user interface, happens from its perspective. Changing the design to having functions or other classes that consume a node don't solve the problem, because then it is definitely not "discoverable". To me it seems that it is therefore almost unavoidable that it has many methods. Wherever possible it is fine to group them and expose them in some subnamespace, as with the repository methods. But this can have downsides in and of itself for "user-friendliness". I am not convinced that just by tab-completing and looking at the methods you can always successfully intuit how a tool functions, especially not one that is as complex as AiiDA, for better or worse.

[sphuber]

Thanks for the review @ramirezfranciscof . I have added the additional tests and included the changes that had snuck into the other PR.

[ramirezfranciscof]

Basically I would point you towards: https://python-patterns.guide/gang-of-four/composition-over-inheritance/#dodge-mixins: Using "Composition over inheritance", is something we do ok in some areas of aiida, but poor in others

This is actually a super interesting concept I came across somewhat recently while reading Head First Design Patterns (highly recommended if you can get past the silly tone 😅). I would find it really exciting to have these kind of design concepts come up more when discussing and deploying implementations and refactors in the code!

More practically, it would also be very helpful when doing improvements like #4938 if there was a bit more explanation on how these kind of principles affect the decisions on making the specific changes, and how these influence the technical aspects of the implementation. I mean, I understand these might seem obvious being the author, but it is sometimes very hard to just look at the changes and deduce what one is trying to achieve, specially since in implementations these concepts can't be usually applied in "pure" form. For example, while it is true that the NodeRepositoryMixin is technically being applied through inheritance, I don't think it is purely that, since the repository is being composed into the mixin (kept in the _repository_instance variable). So in a way is like using composition, but further abstracting and separating all methods related to this composition from the class. Of course one would then argue that the point of the composition is not adding extra methods to the class, to which the retort would be that the extra methods serve as "interface shortcuts" for the users (which I think is essentially the summary of the short exchange above). But I guess my point is that (only?) having that discussion in the abstract can soon become either circular, repetitive, or aimless, and can make it hard to turn into something actionable.

In any case, it is true what @sphuber says that this is more a long term concern and not something we can resolve before 2.0, so maybe we can continue this discussion in other more specific issues and accept for now that this PR will have to construct on what is already there.

[ramirezfranciscof]

This is good to go for me! (it is out-of-date though, not sure if that means you need approval once again after updating)