-
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 command verdi presto
#6351
Conversation
3435a0a
to
d79ffc7
Compare
I have added a commit on top that now automatically tries to detect a RabbitMQ server on localhost. If it finds it, the profile is configured with that as broker, otherwise the profile will have no broker. The second thing I could still add is to automatically detect a Postgres server, and if it exists automatically try to create a role and a database and use |
This is great, and I think very reasonable. I think a user will quite quickly want to submit calculations after starting to use AiiDA. One comment might be that at some point we'll probably have multiple brokers. But I think in the foreseeable future the RabbitMQ broker will be the preferred option if it is available.
I would like this, but I'd say it's less necessary than the RabbitMQ broker. I'll give it some more thought while I'm dogfooding! 🐶 |
Honestly, I don't think this will be very likely. I think it will be a lot more complicated than having different storage plugins and I see little benefit. Up until now, I think pretty much all users only ever used the default configuration as well as technically there wasn't really an API to customize it (although they could have manually updated the config json). So I think this will go a long way and time for over 99% of the use cases.
I gathered as much from the meeting that people are still relying on this |
I would be inclined to agree, if not for the well-known issue we have with RabbitMQ, where after setting up the profile using
But that's a fight for another day. ^^
I agree with this approach. Regarding the issue of adding database-specific options: I think we should avoid them. The command should try to figure out sensible defaults as much as possible, and only prompt the user in case it needs to, even with flags. |
Fair, but this is for users who are already going through the trouble of installing RabbitMQ, so then I think it is fine to have this warning and point them to instructions to get rid of it. This won't affect the user that is just going to do |
Some initial dogfooding comments:
|
Makes sense to me, happy to adopt this terminology.
Fine as well. Do we keep the email though? We had a long discussion some time ago and since the email is not mutable, maybe we should keep that option for people making a production profile with |
Hmm, right. Maybe it's easier to reconfigure your existing user than creating a new one and then updating the nodes somehow. That said, some dogfooding of the
Note how it only complains about the extra argument at the end. I now understand that I have to do
But that doesn't seem that intuitive, especially since other CLI commands we have typically don't need to specify the identifier of the object you are operating on as an option. Beyond the scope of this PR though, but happy to write this down in an issue if you agree this should be changed1. In the end, I'm not sure if we should add the Footnotes
|
First of all, this is terribly off-topic. You put that thing back where it came from or I will be forced to whip out my Mr. ScopeCreep image again. That being said, the confusion comes from the fact that the same command is being used to create new users and update existing ones. On top of that, I agree that one might expect the email as a positional argument. Finally, the error of the superfluous additional positional argument only being thrown at the end of all the prompts is a side effect of clicks parsing order. This is not unique to this command but will happen for all interactive commands. I even remember an ancient issue about this. Edit: found it #1938 |
Back on topic, I would add no options for now, until we figure out the user issue some more. But I'm not so against it I would put my foot down. Happy either way. |
Just to provide some context as to why the concept of users exist. From the very beginning when the database schema was created, it was decided that each node (and other entities) should have an "owner". So the |
Thanks @sphuber, I remember a similar explanation when I was complaining about the User concept in the past. ^^ One thing that I was wondering about re-reading this: what happens if a user tries to import nodes from another I'm pretty sure I've run into this already, but with the new |
Correct. The user table doesn't have a UUID but the email has to be unique. The assumption was made that since emails are typically unique identifiers for people in the real world, it would work for AiiDA as well. |
More dogfooding! 🐶 Default profile names after the firstThis one is a bit minor, but just a thought. After making a first profile, I wondered what the name of a second profile generated with
I like that the hash is shorter (compared to the massive database names Default
|
That would work for me as well.
Since this is the scratch directory essentially, I thought it made sense to have it in a temporary directory that makes it clear that it is scratch. The directory is determined by
I think this makes more sense indeed. But, besides breaking backwards-compatibility, we would have to check carefully whether changing the working directory on a computer that is actively being used could have unexpected effects. Some code may silently assume that this is immutable. |
To give a summary of points made by @mbercx that are directly pertinent to this PR and not wildly scope-creepey:
I am onboard with all suggestions, except I would keep the |
How temporary is a temporary directory? I'm worried that users might expect to be able to use e.g. |
@sphuber @mbercx feel free to tag me for review once you converge, happy to take a look and do a bit of dogfooding.
Yes please! |
4e535ef
to
a509516
Compare
I have addressed the final action points of this comment #6351 (comment) This is now ready for final review. Pinging @danielhollas |
0eaf61b
to
08a933d
Compare
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.
Smattering of minor nits and suggestions. My main concerns:
- Should this be better documented throughout the documentation (tutorials and such). Okay to be done in separate PR so we don't block it here and can start using it.
- Would like to see more end-to-end test.
I'll be testing this hopefully early next week. Exciting! 🎉
|
||
Usage: [OPTIONS] | ||
|
||
Set up a new profile as automatically as possible. |
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.
This sentence sounds strange to me. Maybe...
Set up a new profile as automatically as possible. | |
Set up a new profile without a fuss. |
Or
Set up a new profile as automatically as possible. | |
Set up a new, pre-configured profile. |
Set up a new profile as automatically as possible. | |
Quickly set up a new profile. |
Not quite happy with any of these, perhaps @mbercx will have the right idea.
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.
Also found it difficult to find a good phrasing for this. Agree that what I have currently is unsatisfactory
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.
Given the name of the command, perhaps something a bit light-hearted might actually work. :-)
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.
Given the name of the command, perhaps something a bit light-hearted might actually work. :-)
Yeah fair enough, how about
Set up a new profile in a jiffy.
Not sure how well known that expression is to non-Brits though
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 love in a jiffy
! But as you mention, it probably might confuse some people. And given presto
itself might not be known to everybody (sorry italians 🇮🇹 ) we better be clear in the first sentence of the docstring.
I asked a hippo for help, how about
Set up a profile in no time.
Set up a profile without hassle.
Set up a profile rapidly/swiftly and without hassle.
The last one tries to convey that it should be both quick and easy (without a fuss). But fuss or hassle might have the same problem as jiffy? Okay I'll stop painting the bikeshed now. 🚲
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.
Hey, that's why we are here, to bikeshed until we collapse. I will keep jiffy for now and let's see what the others think
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.
It's an excellent question which clearly requires extensive debate. ^^
I think one issue in properly describing the command is that it doesn't set up a barebones profile because it does check for a RabbitMQ broker, but it doesn't check for all services since for PostgreSQL that is more complicated.
However, the most important aspect of the verdi presto
command is that it's fast. So I'm in favor of something like:
Set up a new profile in seconds.
The second aspect is that it doesn't require user input (for now). So we could add "automatically":
Automatically set up a new profile in seconds.
I'm also fine with "in no time", "without hassle", "in a jiffy". But I think "in seconds" is the easiest to understand for non-natives?
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 could also use "in a matter of seconds" or "in a few seconds". But maybe we should block a 3 hour Zoom call to discuss this at length... 😁
08a933d
to
c0d9b66
Compare
@sphuber thanks for indulging me, and improving the tests! I left a couple of follow up comments. I have also tested this locally and it works great! 🎉 |
5333c5b
to
76fc768
Compare
My pleasure, intensive reviews always improve the end result. Your time is much appreciated! |
|
||
\b | ||
* Create a new profile that is set as the new default | ||
* Create a default user for the profile (email can be configured through option the `--email` option) |
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.
* Create a default user for the profile (email can be configured through option the `--email` option) | |
* Create a default user for the profile (email can be configured through the `--email` option) |
the profile if found. Otherwise, the profile is created without a broker configured. This _does_ mean, however, that | ||
not all functionality of AiiDA is available most notably running the daemon and submitting processes to said daemon. |
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.
the profile if found. Otherwise, the profile is created without a broker configured. This _does_ mean, however, that | |
not all functionality of AiiDA is available most notably running the daemon and submitting processes to said daemon. | |
the profile if found. Otherwise, the profile is created without a broker, in which case some of the AiiDA functionality will be unavailable, | |
most notably running the daemon and submitting processes to said daemon. |
76fc768
to
3eff875
Compare
Hey guys, sry, a bit late to the party here. Just went through the discussion and the code changes, and don't really have anything to add after the excellent comments by @mbercx and @danielhollas. Seems in very good shape to me! |
Ironically, this bike shed is taking ages to paint. 🤣 |
This command aims making setting up a new profile as easy as possible. It intentionally provides a limited amount of options to customize the profile and by default does not require any options to be specified at all. For full control, the command `verdi profile setup` should be used instead. The main goal for this command is to setup a profile with minimal requirements to make it easy to install AiiDA and get started as quickly as possible. Therefore, by default, the created profile uses the `core.sqlite_dos` storage plugin which does not require any services, such as PostgreSQL and RabbitMQ are not required. This _does_ mean, however, that not all functionality of AiiDA is available, most notably running the daemon and submitting processes to said daemon. The command performs the following actions: * Create a new profile that is set as the new default * Create a default user for the profile (can be configured through options) * Set up the localhost as a `Computer` and configure it * Set a number of configuration options with sensible defaults In the future, it may be possible to incorporate the functionality of the `verdi quicksetup` command that automatically creates the role and database in PostgreSQL necessary for a profile with the `core.psql_dos` storage plugin. This would allow `verdi quicksetup` to be retired leaving just `verdi presto` and `verdi profile setup` to provide all the profile setup needs.
3eff875
to
6f41143
Compare
Ok, I think we have bikeshedded enough for now. I am merging this. If there are still important changes, they can be made before the release. |
This command aims making setting up a new profile as easy as possible. It intentionally provides a limited amount of options to customize the profile and by default does not require any options to be specified at all. For full control, the command
verdi profile setup
should be used instead.The main goal for this command is to setup a profile with minimal requirements to make it easy to install AiiDA and get started as quickly as possible. Therefore, by default, the created profile uses the
core.sqlite_dos
storage plugin which does not require any services, such as PostgreSQL and RabbitMQ are not required. This does mean, however, that not all functionality of AiiDA is available, most notably running the daemon and submitting processes to said daemon.The command performs the following actions:
Computer
and configure itIn the future, it may be possible to incorporate the functionality of the
verdi quicksetup
command that automatically creates the role and database in PostgreSQL necessary for a profile with thecore.psql_dos
storage plugin. This would allowverdi quicksetup
to be retired leaving justverdi presto
andverdi profile setup
to provide all the profile setup needs.