aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
411 stars 184 forks source link

Docs: Rework the installation section #6455

Closed sphuber closed 16 hours ago

sphuber commented 3 weeks ago

The verdi setup and verdi quicksetup commands have been deprecated and replaced by verdi profile setup and verdi presto. The installation docs were heavily outdated and the flow was scattered.

The biggest change is that there now is a "quick install guide" that relies on verdi presto to provide an install route that is fool proof and will work on almost any system in a minimal amount of commands.

Then there is the "complete installation guide" that provides all the details necessary to fully customize an installation.

sphuber commented 2 weeks ago

Thanks for the review @GeigerJ2 . I have accepted most of your suggestion. I have given more extended responses to your questions/suggestions related to the current structure of the docs.

khsrali commented 1 week ago

Just one more quick comment: On the bottom of page "Complete installation guide", the Next button navigates to Docker page, I understand this is done automatically by sphinx, but cannot we direct it to Tutorials instead? to me this seems more relevant.

sphuber commented 5 days ago

I think we may need a meeting to discuss in person, because going back-and-forth on github doesn't seem very efficient.

I think my initial gut reaction towards seeing the --postgres-X options was more that I wanted to keep verdi presto "clean" and "simple", without adding too many options

I fully agree. And the only options that I added are those that are necessary.

I also was wondering what we should do if we add another storage backend in the future that might need options.

We shouldn't. In my opinion, we are adding this functionality just for the core.psql_dos for historical reasons to keep the functionality that was originally in verdi quicksetup. I don't think that we should cater to needs of other backends, for example, some of the plugins of aiida-s3 that connect to an object store. I don't think we should have to implement custom logic that makes it easier or automates the creation of the required resources, such as the containers/buckets. That is up to the user.

The main reason I started using verdi quicksetup again was because I could specify the database name, which allowed me to easily identify the database when I wanted to do anything directly in PostgreSQL (e.g. for backups, hacking codes/computers, checking DB size, ...). So one use case for being able to specify the database name is for more advanced users that like the convenience of verdi presto but still want to have easily identifiable database names.

This seems to be contradictory. On the one hand you want to keep verdi presto simple, but then you want to start adding options that just happen to be useful to your particular use case. Can't have your cake and eat it too my man :D

The main reason I started using verdi quicksetup again was because I could specify the database name, which allowed me to easily identify the database when I wanted to do anything directly in PostgreSQL (e.g. for backups, hacking codes/computers, checking DB size, ...)

Just run verdi profile show and the database name is there. I don't think this "hurdle" justifies going against the principle of keeping verdi presto as simple/lean as possible.

Another use case was users that can't create databases themselves, but have an admin who creates those for them (there were 2 users in ETH who were faced with this problem). Interestingly, verdi quicksetup also helped here, since if you specify an existing database, it simply warns that the database exists but still uses it.

If the database already exists, why use verdi presto/quicksetup then? Just use verdi profile setup core.psql_dos. Why are we complicating things?

The --repository-uri could also have the same default as verdi presto

This should be easy to fix.

and should probably be easier to specify (now you need to prepend it with file://, which is not very user-friendly).

The file:// protocol prefix is historical and, I agree, unfortunate. We could change it by removing it, but it would require a config migration. Might still be worth doing because it has been annoying me for years. It was added I believe in the very beginning to allow in the future changing protocols for the file repository, but this is now properly addressed through storage plugins, I would say.

But is it really necessary? If --use-postgres is set and the database creation fails, the user is prompted for the required information:

I don't think relying on prompts is good design. At least it should not be required. There should always be a non-interactive option. I don't see the problem though. The options are exactly that, optional. They only show up if the user looks at the help text. How is that "confusing"? I could see the point with the old setup commands where all options would always be prompted for, even if users often would just want to use the results. But here, they are not even prompted for it. They just show up in the help text. I don't see a problem with that if it means that it provides all necessary options for all potential variants of PSQL servers.

mbercx commented 5 days ago

We shouldn't. In my opinion, we are adding this functionality just for the core.psql_dos for historical reasons to keep the functionality that was originally in verdi quicksetup

Makes sense, I agree. 👍

Can't have your cake and eat it too my man :D If the database already exists, why use verdi presto/quicksetup then? Just use verdi profile setup core.psql_dos. Why are we complicating things?

Haha, completely fair. It's also why I argued that in the end making the usage of verdi profile setup core.psql_dos smoother was the way to go for these use cases. Perhaps I should have led with that. ^^

I don't think relying on prompts is good design. At least it should not be required. There should always be a non-interactive option. I don't see the problem though. The options are exactly that, optional. They only show up if the user looks at the help text. How is that "confusing"?

Perhaps, we are probably a bit too hung up on these extra options in our zeal of keeping verdi presto minimal, making a mountain out of a molehill. One non-interactive alternative would be to use verdi config to set the postgresql defaults for an AiiDA instance (which could even at some point be overridden globally), and then these are automatically picked up by verdi presto. But having to execute two commands is also not ideal (though that's also true in case the user has a non-standard RabbitMQ configuration). Another argument might be that for any non-interactive use case, verdi profile setup core.psql_dos could be used. But in the end, I'm starting to feel more and more obstinate as I'm writing this, so maybe it's best to acquiesce and move on. ^^