Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The aiida_local_code_factory fixture is badly named #5214

Closed
sphuber opened this issue Nov 2, 2021 · 5 comments
Closed

The aiida_local_code_factory fixture is badly named #5214

sphuber opened this issue Nov 2, 2021 · 5 comments

Comments

@sphuber
Copy link
Contributor

sphuber commented Nov 2, 2021

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.

@ltalirz
Copy link
Member

ltalirz commented Nov 5, 2021

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.

@sphuber
Copy link
Contributor Author

sphuber commented Nov 5, 2021

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.

@ltalirz
Copy link
Member

ltalirz commented Nov 5, 2021

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

@ltalirz
Copy link
Member

ltalirz commented Feb 18, 2022

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

@sphuber sphuber self-assigned this Apr 21, 2024
@sphuber
Copy link
Contributor Author

sphuber commented Apr 21, 2024

This was fixed by #6341

@sphuber sphuber closed this as completed Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants