-
Notifications
You must be signed in to change notification settings - Fork 192
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
CLI: Add the verdi init
command
#6315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I think this makes a lot of sense, and seems slightly preferable to #6305.
verdi presto
seemed a bit too close too verdi quicksetup
as somebody already noted (certainly one cannot tell the difference by name). Perhaps some of the ideas from there could be ported to quicksetup
via a new flag?
I like this approach because it is kind of like a virtualenv in Python versus installing stuff globally. Now I can have completely separate .aiida
in every AiiDA-related git repo, which will be very nice for running tests, where right now we need to rely on pytest fixtures to clean after themselves.
(I didn't do a thorough review, mostly just read through the documentation).
90eb637
to
5dd0216
Compare
There are still some questions about desired behavior of the command:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sphuber this is great! I add a few comments.
assert dirpath_loaded == dirpath_config, f'Filepath of loaded config `{dirpath_loaded}` != `{dirpath_config}`.' | ||
|
||
if from_archive: | ||
storage_backend = 'core.sqlite_zip' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can a user switch the backend to sqlite_dos
later easily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they cannot. But why would they want to do this? If they actually want to use the data in a profile that is mutable, they can simply do the following instead:
verdi init
verdi archive import archive.aiida
I would say that is simple enough as an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a user case: users import an archive using sqlite_zip, then after inspecting the archive, they find it's good to start work directly on top of it, and if there is a simple way to quickly switch the backend, that would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot really convert the core.sqlite_zip
backend to an core.sqlite_dos
one. What we would have to do is just create a new profile from scratch with core.sqlite_dos
and then import the archive. The command to do this would have to be documented, but in that case I think it is just easier to tell users to do verdi init && verdi archive import
.
I think the current behaviour is fine, after all the user can simple change a directory, right? Are there any use cases where this would potentially be undesirable? I fear anything we'd do otherwise would be confusing / unintuitive. E.g. if the init profile is created next to an existing profile, should it still be made default? |
I agree. However, @giovannipizzi suggested that the current syntax for creating a new profile in an existing instance, e.g. |
5dd0216
to
25de73a
Compare
I have addressed the comments. Does anyone still have major comments? Otherwise I propose we merge this so we can start dogfooding this. We will wait with the release for a while so we can still further fine tune it. I will also start working on improving the docs to include this new method of getting started with AiiDA in an easy way. But will leave that for a separate PR. |
Hi @sphuber, I am not clear how to use this feature. Suppose I create a folder and run
Maybe this will be clear in the docs PR? |
With #6322 AiiDA will now check the current working directory for an
You should see that it now uses the local
Exactly as normal with
They define separate AiiDA instances that are completely isolated. See the docs: https://aiida.readthedocs.io/projects/aiida-core/en/latest/howto/installation.html#isolating-multiple-instances |
I got a problem (aiida) xing@thinkpad:~/tests/aiida/test-6315$ verdi init
verdi status
verdi profile list
verdi profile show
Success: Initialized new AiiDA instance in `/home/xing/tests/aiida/test-6315/.aiida`.
Report: Initialising the storage backend.
Report: Storage initialisation completed.
Success: Created new profile `init`.
Success: Configured the localhost as a computer.
✔ version: AiiDA v2.5.1.post0
✔ config: /home/xing/.aiida/envs/aiida/.aiida
✔ profile: default the config still in $ echo $AIIDA_PATH
/home/xing/.aiida/envs/aiida thus, the |
You are right. The The question now is what the behavior should be. This command is intended for beginning users of AiiDA, so they are unlikely to have defined Actually, should we even allow to specify the path?
The second command will not actually work against the just created instance. I think I will remove it and just get the path from |
25de73a
to
724d4bd
Compare
We need to think about how to handle the global verdi and local verdi. As a user, even though I set the btw, this is not a problem for |
Fair point. That means the logic of #6322 should be inverted. That the current working directory takes precendence over |
yes, but I think it's not [Edit] a problem, but an expected behavior. The user want to change the AiiDA config when going to a local AiiDA folder and its child folders. |
I am also leaning towards local .aiida having preference, seems more intuitive. Another perhaps more general question, how does this interact with verdi daemon? Can there be more isolated daemons running? Or is there a one global instance that can handle different .aiida folders? |
It is fine by me to change to this behavior, I am just a bit worried about breaking behavior. People that have been relying on
Currently, you can have multiple instances running alongside one another just fine. Each daemon is perfectly isolated. The |
It is not so trivial to just reverse the precedence of CWD and AIIDA_PATH to determining the config dir. As soon as a config dir is created in the users home folder, that will almost always take precedence over Removing the automatic creation of Leaves perhaps to ignore Hypothetically, even if we disregard backwards compatibility just for the time being, what would the ideal most intuitive behavior be? Seems that having the "global" config in the home directory may be complicating things, but if we get rid of it, it will force users to always manually run something like So, thoughts? |
My though:
|
Hmm, tricky. I don't think erroring out is very user-friendly, rather we should have a predictable behaviour.
Tbh although this feels hacky, it seems like a best option for now and ia well defined. I assume that most (beginner?) users don't use AIIDA_PATH, so this will not affect the behaviour for most, and at the same time we will not break advanced users using AIIDA_PATH. If they are already customizing their aiida path, it would seem they will have less incentive to use the new init command anyway. Thinking in abstract, Essentially we have similar situation as in python, i.e. using venv versus global install. So we could potentially have some kind activation script in the future. But that seems like an extra complication for now. Let's dogfood it for a while and see? |
Hi all, very interesting feature and discussion! For the use case of quickly exploring an archive with a temporary profile without polluting the main one, this works perfectly, and I love the idea! To me:
also seems like the best solution to this conundrum so far. I learned about Some other points:
But: verdi init
verdi archive import <local archive/URL> should be simple enough, really... Though, there are a few more thoughts that I'd like to bring up here, which I also mentioned briefly in the last As In the end, in this PR, I think we are trying to achieve two separate goals:
(And maybe 3: Having a profile for just exploring an archive without importing - though, that seems to tie in with point 2, just using One could add more customization options to So I'm wondering if, rather than now adding For example:
The Effectively, it all boils down to where we put the logic/complexity for the different possible ways to set up an AiiDA instance: As separate I think all of this will be good to discuss as part of the upcoming |
724d4bd
to
eb0cc05
Compare
This command initializes a new AiiDA instance in the current working directory. It then creates a profile using the `core.sqlite_dos` storage backend and no broker plugin, and configures the localhost computer.
The current order of priority would have `AIIDA_PATH` always override the current working directory hierarchy in determining the location of the configuration directory. This would essentially render the CWD logic pointless as soon as `AIIDA_PATH` is defined, which is the case for many existing installations as that was the only way up till now to control the location of the configuration directory. It also breaks the functionality of `verdi init` as the created directory won't be picked up. Therefore, the order is switched and the CWD takes precedence. The default config directory location, which is currently the user's home folder, is hereby ignored when going up the hierarchy. This is because this folder is likely to almost always exist and be present in the CWD hierarchy and would therefore always override the `AIIDA_PATH`.
be385d0
to
d35861b
Compare
Thanks a lot @sphuber ,
I would have expected that after |
Totally agree. Was just awaiting the discussion on what we will do with the
There is no |
Before starting with proper dogfooding, I already have some conceptual comments I wanted to make, mostly on 1059b5f. Let's start with some semantics: What exactly is an AiiDA instance? Interestingly, the definition of this concept was sublty changed in 1059b5f. Before, the documentation said:
This is changed in 1059b5f:
So, the source code is no longer part of the definition of an AiiDA "instance". Indeed, the changes in 1059b5f allow for multiple AiiDA configuration folders ("instances" in the new definition) for one Python environment and AIIDA installation. I understand the problem the commit is trying to solve. Once a user starts having multiple AiiDA installations, they needed to use However, I'm not sure if the new definition of an AiiDA instance makes sense to me. Does it make sense to have multiple AiiDA instances for one Python environment/AiiDA installation? What advantages do multiple separate instances have over multiple profiles in the same instance? If not, perhaps we should look for an alternative approach to properly separate the "old" version of AiiDA instances (code and configuration) instead. I'm already worried about a few things:
So, I'm mainly wondering if we should not revert 1059b5f in favor of a different approach to separating AiiDA instances that respects the original definition: AiiDA source code/install and configuration. Footnotes
|
@mbercx some really insightful comments! The fact the this PR changes the semantics (not only behaviour) is a really great observation. But I would like to turn this around back at you. :-) The fact that you needed to start
I think this is a key question to answer. I wonder if/how people use different profiles? I don't have too much experience with them, but to me the concept is very opaque. At least from the command line, working with different profiles is kind of awkward, since you need to always provide an extra argument for non-default profiles, right?
I guess I agree, with the caveat that with should discourage users to muck around with AIIDA_PATH and prefer .aiida path-based git-like discovery if we go forward with this. To me the git-like model makes things quite a bit easier to wrap my head around. Different projects are simply different directories. If you for some reason need different aiida installation for different projects, you could put Python venv ( Besides the |
To me, different profiles allow me to have multiple storages in (what I call) the same project. A typical use case would be a
You can use
It's interesting to see the different perspectives here. ^^ Due to AiiDA's heavy reliance on plugins, I can't imagine having the same Python environment for two different projects. If for some reason you need to have multiple datasets in the same AiiDA installation, you can make a new profile. I kind of get the feeling you want to use the new isolated AiiDA folder instances in lieu of AiiDA profiles, in a sense? I suppose it then boils down to what you prefer. I like being able to have a Jupyter notebook anywhere and select the kernel to be able to interact with different AiiDA instances (old skool definition), and then load the profile whose dataset I'm interested in. Having to copy or move the notebook to interact with a different instance seems tedious.
Yes, I see, interesting point. Though I think most Python environment tools allow you to execute commands with that environment independently of which folder you are in. I suppose what I was loosely suggesting:
Is sort of similar to I'm still more in favor of not separating the AiiDA config from the AiiDA installation/Python environment though. Another argument is that the config format is actually dependent on the PS: we're actually meeting on this topic at 9:30 CEST today (so in less than 7 hours ^^). I'll send you the Zoom details in case you want to join. |
Yes yes I understand, but still, even if I want to What I mean, is that I expected it behave the same both ways. Either deleting the folder manually or |
But that is the thing, |
From @khsrali's comment above you can already see that this new approach is causing confusion. The problem is that |
I'm writing a small essay to continue from our discussion last Friday, but already wanted to note this before I forget:
It seems the |
After our meeting on Friday, I took a bit of time to write down the discussion and supplement it with some further thoughts. Buckle up and grab a fresh cup of ☕, because this is going to be a long one. SemanticsI'll start with some semantics again, to limit confusion in the presentation and the discussion that follows. I will not define an AiiDA "instance", since this definition is exactly what is up for debate. For now, I will define1:
The image below tries to visualise and connect these concepts: IntroductionAs @GeigerJ2 mentioned, the
(1) would be fully satisfied by " (3) is the main issue I want to discuss, i.e. the changes made in 1059b5f. I could also post my essay there, but I think The Status QuoThe current definition of an AiiDA "instance" is straight from the documentation:
It continues:
Note how it mentions to store the configuration in a virtual environment. Next it mentions using Python environments, which I'm sure we all agree with:
Then comes the tricky bit:
This is the current problem with separating AiiDA instances. By default, the
But this requires setting the
|
Thanks for the very detailed writeup @mbercx . I have read through it and agree with most things and as I mentioned during our last discussion I am leaning to agreeing with you that we should probably revert 1059b5f . The idea was nice, but with the current status quo it introduces too many situations where the behavior of AiiDA or the "best" approach to things become unclear. That being said, here are some short comments I wanted to make:
@giovannipizzi @danielhollas the ball is now in your court I believe. The other people that were present during the discussion seem to be largely onboard with the analysis presented above and to remove the directory-based discovery of the config directory. Could you please let us know if you agree or if you have counter arguments that you think deserve to be discussed first. Ideally I would like to take a discussion soon because we would really like a |
Thanks to @giovannipizzi, @sphuber, @danielhollas, @GeigerJ2 and @khsrali for the discussion! I'm going to summarize some points here:
Action points:
Also thanks @sphuber for your comments here. I'll definitely consider/integrate the first in my AEP. |
But |
Whoops! You're right. So then should |
That's the point I was trying to make during the discussion: determining what the right behavior is, can take forever. You can always find an example where a user has a use case where the alternative behavior feels more intuitive to them. All of this doesn't matter for the discussion on this PR though, as this is the status quo and |
My only fear is that if we now make also the new command change the default profile, this is not something easy to change in the next release. This new command is for a different set of users who might have different expectations, and being new we can decide if the default behavior should be different |
The problem is: based on what do we make this decision? As @sphuber mentioned, we don't really know what which user expects. Frankly, I think it's pretty intuitive that in case I make a new profile from the command line, I want to start working with it. If we don't have a solid motivation to change the behaviour compared to the other profile setup commands, keeping the behaviour consistent seems like the better plan. |
I think the main point is that you wouldn't want other running shells that were using the previously default profile to switch to the new one and fail. |
Closing this in favor of #6351 |
This is an alternative solution to #6305
The
verdi init
command is slightly different in that it doesn't just create a new profile, but it creates the profile in a new AiiDA instance, i.e., a new.aiida
folder is created (in the current working directory by default). This makes the behavior similar togit init
that initializes a new repo. Once the user is done, the instance folder can simply be removed and there will be no trace left. By default the command will create a profile with thecore.sqlite_dos
which won't require any services, but alternatively a profile withcore.sqlite_zip
can be created instead from an existing archive. This version will be read-only, but will allow a user to easily start browsing an archive with a single commandverdi init --from-archive some_archive.aiida
.