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

verdi setup: Add the --profile-uuid option #5673

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 27, 2022

Fixes #5670

When setting up a profile in multiple AiiDA instances that should all use the same services (storage backend, message broker), for this to work, the profile UUID should be identical. Currently, there is no way to set the UUID of a profile through the CLI. The verdi setup and verdi quicksetup commands will always generate a new random UUID.

Here we add the --profile-uuid option to verdi setup which allows to specify the desired UUID instead of having one generated. It is not added to verdi quicksetup because in this use case of sharing resources between profiles, the resource is normally created before hand. Likewise, this use case is often deployed automatically which uses verdi setup in a non-interactive way. This is why the option will not be prompted for in interactive mode. That would only confuse normal users that are setting up a normal profile where the UUID should simply be generated automatically.

@sphuber
Copy link
Contributor Author

sphuber commented Sep 27, 2022

@ltalirz this change came up during my experimentation of having multiple AiiDA instances configured to use the same services. So far, this seems the only change that would make this use-case easy to setup.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sphuber !

just one nitpick that may be good to address

'same services of a profile that is already configured in another AiiDA instance, in which case the profile UUIDs '
'need to be identical.',
required=False,
type=str,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should validation be added here?

some code might rely on the profile uuid actually being a uuid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but the API actually also doesn't have any validation. Any string will be accepted and will also work. Well, I am not sure if any string would work, since it is used for the RabbitMQ resources and that might put restrictions on it.

It would maybe be good to add validation on the Profile class on what is acceptable for the profile_uuid key. But until that isn't there, I don't think we should put validation on the CLI. I can add this in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ltalirz you ok with merging this as is? I can open an issue to address the validation of the profile UUID more generally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, go ahead

@chrisjsewell
Copy link
Member

chrisjsewell commented Sep 28, 2022

When setting up a profile in multiple AiiDA instances that should all use the same services (storage backend, message broker), for this to work, the profile UUID should be identical. Currently, there is no way to set the UUID of a profile through the CLI. The verdi setup and verdi quicksetup commands will always generate a new random UUID.

@sphuber my only worry here, is that you are "silently" adding another design goal/constraint to AiiDA: essentially that multiple users, on separate machines, should be able to use a single profile.

Not to say this isn't a good goal, but where is this documented, i.e. where is the AEP? and have you considered other things like say, what if two profiles have the same UUID, but point to different postgres databases, or have other configuration differences, and also what are the "new" design contraints for an alternative to rabbitmq?

I really don't feel that the UUID should be exposed, until these kind of questions have been answered 😬

@sphuber
Copy link
Contributor Author

sphuber commented Sep 28, 2022

I really don't feel that the UUID should be exposed, until these kind of questions have been answered grimacing

A fair point, but the thing is, it is already exposed through the Python API. Just not through the CLI. People can already freely change the UUID through the Python API or even directly in the config.json. But I see that that is not necessarily a reason to expose it even further.

Nevertheless, in v1.0 we did actually design everything with this use-case in mind. If you think about it, each daemon worker is in essence an individual process that is using the same services (Postgres, RabbitMQ, filesystem). Really the use-case that I am describing is nothing more than multiple daemon workers that just happen to run on different machines. It could very well be that there are some edge cases in the code that do not work well with this mode, but that is exactly what I am experimenting with. Currently it is just really difficult to deploy multiple machines with the same config. Adding this option solves this problem.

I agree that adding this option should not impact normal profile setups though, which is why I made sure to not have it prompt. This way it should never be an issue for users. What I could do additionally is "hide" the option, such that it doesn't even show up in the help info of the verdi setup command. As long as the use-case is not yet documented and officially supported, I think this would make perfect sense.

@chrisjsewell
Copy link
Member

What I could do additionally is "hide" the option, such that it doesn't even show up in the help info of the verdi setup command. As long as the use-case is not yet documented and officially supported, I think this would make perfect sense.

Yep that sounds great thanks

@sphuber sphuber force-pushed the feature/5670/verdi-setup-profile-uuid branch from 97aae53 to cbe4f5b Compare September 28, 2022 14:15
@sphuber
Copy link
Contributor Author

sphuber commented Sep 28, 2022

Yep that sounds great thanks

Done

When setting up a profile in multiple AiiDA instances that should all
use the same services (storage backend, message broker), for this to
work, the profile UUID should be identical. Currently, there is no way
to set the UUID of a profile through the CLI. The `verdi setup` and
`verdi quicksetup` commands will always generate a new random UUID.

Here we add the `--profile-uuid` option to `verdi setup` which allows to
specify the desired UUID instead of having one generated. It is not
added to `verdi quicksetup` because in this use case of sharing
resources between profiles, the resource is normally created before
hand. Likewise, this use case is often deployed automatically which uses
`verdi setup` in a non-interactive way. This is why the option will not
be prompted for in interactive mode. That would only confuse normal
users that are setting up a normal profile where the UUID should simply
be generated automatically.
@sphuber sphuber force-pushed the feature/5670/verdi-setup-profile-uuid branch from cbe4f5b to 6ecfc16 Compare September 28, 2022 15:02
@sphuber sphuber merged commit 51e0f9a into aiidateam:main Sep 29, 2022
@sphuber sphuber deleted the feature/5670/verdi-setup-profile-uuid branch September 29, 2022 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

verdi setup: add option to set the profile UUID
3 participants