aiida-core icon indicating copy to clipboard operation
aiida-core copied to clipboard

The `aiida_local_code_factory` fixture is badly named

Open sphuber opened this issue 2 years ago • 4 comments

It actually creates a "remote" code and not a "local" code. It probably was named this because the "remote" computer is localhost, but it is confusing nonetheless. I wanted to make a similar fixture to create a local code, but that would clearly lead to confusion. I would change to one of the following:

  1. Rename it to aiida_code_factory and make the type an argument, defaulting to "remote"
  2. Rename it to aiida_remote_code_factory and making aiida_local_code_factory actually create a "local" code

I think option 1) is preferable since it also allows to easily deprecate the current method. Only downside is that I am not sure what the type argument should take as a type. The simplest would be a simple string "remote" or "local" but that is kind of ugly. An enum would be better, but then you would have to import this again in order to change the default.

sphuber avatar Nov 02 '21 21:11 sphuber

I would go for aiida_code_factory with string arguments.

In general, the concept of local and remote codes has been leading to naming headaches for a long time. Would there be an outcry if we just dropped support for local codes? Is there an important use case for them?

Edit: We once asked on the AiiDA mailing list and nobody replied

4 years ago I still argued that storing codes in AiiDA could help to preserve provenance... but if nobody uses it this way, then I think we better remove it since it just makes our life more complicated.

ltalirz avatar Nov 05 '21 15:11 ltalirz

Thanks for the info @ltalirz . I feel that a big part of the problems arise due to the design of a single class Code doubling for two quite different objects. I wonder if we were to split this into two separate classes and untangle the code would help with all of this. It would also maybe make commands clearer if we make separate ones as opposed to a single one.

The project on adding support for containerised codes (see this open AEP) most likely will also have to add the concept of a Code and I already proposed to use a separate plugin and give it an entry point string as a child in the hierarchy, such as code.containerised. Maybe it would be interesting to use this effort to also redesign Code and split it in code.remote and code.local. This is something I could look into for the coding week.

If it is possible, it would maybe be better to keep the concept around, but well separated and use v2.0 to allow some potentially necessary breaking changes. We can then have some more time to keep it around (I do think there are some usecases for it outside of materials science) but if it really proves useless, dropping it in the future will be easy.

sphuber avatar Nov 05 '21 18:11 sphuber

Thanks, your suggestion sounds like a very good way forward to me (better than dropping support entirely).

ltalirz avatar Nov 05 '21 18:11 ltalirz

Just mentioning there was yet another case of someone being confused by the naming https://github.com/aiidateam/aiida-core/discussions/5347#discussioncomment-2203310

ltalirz avatar Feb 18 '22 09:02 ltalirz