Automatic code setup: validation and --all option#1222
Automatic code setup: validation and --all option#1222t-reents wants to merge 2 commits intoaiidateam:mainfrom
--all option#1222Conversation
This commit introduces validation checks during automatic code setup, ensuring that only supported executables are provides. Moreover, a new `--all` option has been added to the setup command, allowing users to configure all available Quantum ESPRESSO codes in one go.
|
maybe good to also add |
|
@eimrek Ah thanks! I remember that I actually typed it and some point, but it got somehow lost. |
mbercx
left a comment
mbercx
left a comment
There was a problem hiding this comment.
Thanks @t-reents! I only had a quick look so far, will give a proper code review when we decide on the API. I left one comment with some questions that are slightly beyond the scope of this PR (apologies), but definitely related, and I think will influence the decision that is made here.
I'm still not entirely convinced that adding --all is a good idea. I think the benefit for the user is rather minimal. Probably a lot more time will have to be spent on thinking about the right prepend_text etc than typing the codes you need. Here are some downsides that I see:
- If you add the forgotten
open_grid.x, we'll already be setting up 13 codes.d3hess.xandaverage.xare being added in #1225 and #1226, respectively. If we decide to also support QE executables that are not in this package (also see my comment), we might be looking at close to 20 codes being set up from which a typical user might 3 or 4. Doingverdi code listis going to give quite a few codes. 😅 Of course, it's up to the user, but I'm not sure how many users would really be happy with having all these codes set up, and I don't know if we should encourage such messy configurations. - Added complexity. This is both in the API (where users suddenly get three more options to consider), and our code, which has to deal with all these options and their combinations. The more options/inputs we have, the more careful I am with adding more. It should really be worth it. It's possible this argument is less important if there are standalone reasons to keep the two other options and I have a closer look at the code.
|
|
||
| from aiida import orm | ||
|
|
||
| VALID_QE_EXECUTABLES = [ |
There was a problem hiding this comment.
I think you also forgot open_grid.x here. :)
One issue with this approach is that (besides forgetting to update this list), there might be other codes that are supported in other plugin packages that aren't in this list, but where I think our code setup command would be able to set up the code just fine (e.g. hp.x, possibly others). I suppose we could add those here as well, but then the list for --all becomes even longer and we're adding logic for external packages that we don't depend upon.
I was also going to add epw.x and xspectra.x here as well, but then I realised that their entry points are epw.epw and xspectra.xspectra. So sadly I won't be able to use our beautiful command to set up these codes. 🥲 I'm wondering if it would be worth considering supporting these executables still somehow (e.g. via a mapping), or it's also too messy because they are external non-dependencies.
If we keep this validation, we should adapt the language though: e.g. use "supported" executables instead of "valid". xspectra.x is a perfectly valid QE executable. I'm even a bit worried that for users of Xing's plugin package this won't be confusing: why can't I install xspectra.x with this command?
Fair point! Some questions here:
|
3 participants
Addressing parts of #1156
--alloption. To make this more convenient, I also added the--ignore-missingand--skip-existingoptions. I agree with you @mbercx, that I'd try to keep this as minimal as possible. I figured that--all(and the related ones) make sense. However, I don't really see the need for a dry-run.Since we have the
--alloption now, theexecutablesargument is optional now.Even without the new
--alloption, adding a--ignore-missingmakes sense. I just realised that the command would fail when specifying three executables of which one is missing, which was a bit annoying.