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

making verdi setup & verdi quicksetup more user friendly #606

Merged
merged 22 commits into from
Sep 12, 2017

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Jun 21, 2017

Please see the commit message for the small improvement to verdi quicksetup.

A few more things that are still to do:

  • Currently, verdi setup provides a default email address (aiida@localhost), while verdi quicksetup doesn't. If at all, it should be the other way around, but I guess a default email address is not a good idea in either case as it will mean that everybody uses the default

  • Currently, verdi setup does not ask for first name, last name and institution, while verdi quicksetup does. Again, I think verdi setup should ask for these as well(?)

So far, the procedure for choosing a custom profile name
in verdi quicksetup was a bit complicated:
it first checked whether a profile 'quicksetup' was already present
(which already requires sudo access);
if it existed, the user was asked whether to overwrite the profile
or not. by selecting no the user could then choose a new profile name.

This commit adds a prompt for the profile name in verdi quicksetup.
It is the very first prompt and it already has a default value
(i.e. one can simply press "enter" and doesn't lose time).

This is important in particular for setting up test profiles,
as those need to start with 'test_'.

The default name for the database was changed from
    "aiida_qs_<login-name>" to
    "<profile>_<login-name>"
This ensures that for test profiles the database name also starts with
'test_'.
and reverted default database user name back to aiida_qs_<login-name>
@ltalirz
Copy link
Member Author

ltalirz commented Jun 22, 2017

Ok, I've now realised that verdi setup does ask for name and institution, but only if one modifies the email address from the default one (and only after the profile repository and the database have already been created).

So, would it make sense to remove this default value or is it needed for something?

Also, it seems the prompts for the user-properties in cmdline/commands/user.py don't use 'click' and have thus a different way of handling default values (click shows the default value in brackets in the prompt, while for the user email the default text is simply copied into the input field). Using a consistent way would make it clearer for the user.

@giovannipizzi
Copy link
Member

I think the default email can be removed everywhere.

I agree that the UI should be consistent, indeed @DropD is in the process of converting everything to click (it's in one of the repositories under his account on GitHub) but this requires some time. Anyone is very welcome to help in the conversion process!

@ltalirz
Copy link
Member Author

ltalirz commented Jun 22, 2017

And one more question:
Currently, verdi help quicksetup shows the python docstring of the quicksetup function.
I think the original docstring "Uses click for options..." illustrates that this can lead to dilemmas, since what the person typing verdi help quicksetup needs to see is not necessarily what a programmer of aiida should see.
In particular, the person typing verdi help quicksetup should not only get a description of what the command does but also a dynamically generated list of options.
For the moment, I have simply added instructions on how to get a list of these options to the docstring, but this is not ideal. Any ideas for a better solution?

in addition to showing the doc strings of the functions,
verdi help now shows the dynamically generated documentation
of the 'click' options for verdi quicksetup and verdi setup.

Adding this to other commands is achieved simply by adding
a static '_ctx' method that returns the appropriate context.
@ltalirz
Copy link
Member Author

ltalirz commented Jun 27, 2017

I've added an easy way to have the click options appear in the output of verdi help ....
Furthermore, I have now removed the default email address from verdi setup.
Everything alright so far?

There is a further quirk I ran into: Currently, verdi setup provides a non-interactive mode, where everything is provided via the command line (I wanted to use this to set up a test profile to automatically run the tests after installing aiida).
However, verdi setup isn't able to do this: it can only connect to an existing database.
verdi quicksetup can set up the database, but it doesn't provide a fully non-interactive mode: even, when you provide all the information on the command line, it still asks

The default profile for the 'verdi' process is set to 'quicksetup_ltal': do you want to set the newly created 'test_aiida' as the new default? (can be reverted later) [y/N]: The default profile for the 'daemon' process is set to 'quicksetup_ltal': do you want to set the newly created 'test_aiida' as the new default? (can be reverted later) [y/N]:

So, in my opinion

  • verdi quicksetup should simply be a wrapper to verdi setup, which fills most of the required variables with default values.
  • both commands should be able to set up the database + database user (for verdi setup one might need to enable a switch --create-db that is on by default in verdi quicksetup)

Do you agree?

giovannipizzi
giovannipizzi previously approved these changes Jun 27, 2017
@giovannipizzi
Copy link
Member

Hi Leo!
I approved the PR. Do you want to merge it already or fix the things of your last comment? In the first case, you can already do it (after updating your branch). Otherwise, you can continue committing here.

Regarding the comment:

  • I would add another click option to decide whether the new profile should be the default one or not, so everything can be non-interactive (and have click ask for Y/N if not provided)
  • I agree quicksetup should be a wrapper
  • I agree both should be able to work fully non-interactively
  • I like the idea of having --create-db as an option of setup (and I guess this means moving the DB-creation logic in there from quicksetup)
  • I'm not sure quicksetup can be masde as simple as just giving default values to setup. Quicksetup has to make some choices when trying to create the DB, and might need to have to ask the user for info, passwords, ...

Maybe Quicksetup should just contain the logic for creating the DB, and then it would call the normal setup with the proper options (depending on what it created?)

@ltalirz
Copy link
Member Author

ltalirz commented Jun 29, 2017

@DropD
I have had a look at your repo and it seems you have done some quite extensive reorganisation of the code. Could you please give us a summary of what you did and what the current status is?

I am happy to help with the integration of your work into the aiida develop branch.
We should avoid postponing this merge too far into the future, though - improvements to the verdi commands are needed and we should implement them soon.

when specified, quicksetup will *not* ask whether to override
default profiles for shell/daemon
Finally a way to delete verdi profiles.
Asks user whether to delete database & database user
@ltalirz
Copy link
Member Author

ltalirz commented Jul 17, 2017

Ok, so as discussed I have added a click option for quicksetup to avoid the prompt for making the new profile default in shell/daemon.
The other things we discussed require more rearrangement in the code and have therefore been postponed until @DropD is back.

I have added one more thing that I have been missing, which is "verdi profile delete myprofile".
This is my first time adding a slightly more significant amount of new lines, so please check it and let me know if I should do some things differently.

@ltalirz
Copy link
Member Author

ltalirz commented Jul 18, 2017

I just realised I forgot to delete the file repository.
I guess I should take the path from 'AIIDADB_REPOSITORY_URI' right?
What is the best way to delete this, as in the future the repository may be on object store...

Also perhaps some of the functions I am using here (in particular the functions to manipulate the database) should probably not be inside quicksetup but rather in aiida.common.setup (?)

Example of command line usage:

$ verdi profile list
* default
* test_mynewtest
* test_aiida
* my_test
> tdevelop (DEFAULT) (DAEMON PROFILE)
(aiida_ltal) leopold@tsf-428-wpa-7-059:~/.aiida
$ verdi profile delete test_aiida
Delete associated database 'test_aiida'?
WARNING: All data will be lost. [y/N]: y
Deleting database 'test_aiida'.
Delete database user 'test_aiida'?
WARNING: This user might be used by other profiles as well. [y/N]: y
Deleting user 'test_aiida'.
Delete associated file repository '/Users/leopold/.aiida/repository-test_aiida/'?
WARNING: All data will be lost. [y/N]: y
Deleting directory '/Users/leopold/.aiida/repository-test_aiida/'.
Delete configuration for profile 'test_aiida'?
WARNING: This permanently removes the profile from the list of AiiDA profiles. [y/N]: y
Deleting configuration for profile 'test_aiida'.
@ltalirz
Copy link
Member Author

ltalirz commented Jul 28, 2017

Finished a first implementation of verdi profile delete.
Example command line usage:

$ verdi profile list
* default
* test_mynewtest
* test_aiida
* my_test
> tdevelop (DEFAULT) (DAEMON PROFILE)
(aiida_ltal) leopold@tsf-428-wpa-7-059:~/.aiida

$ verdi profile delete test_aiida
Delete associated database 'test_aiida'?
WARNING: All data will be lost. [y/N]: y
Deleting database 'test_aiida'.
Delete database user 'test_aiida'?
WARNING: This user might be used by other profiles as well. [y/N]: y
Deleting user 'test_aiida'.
Delete associated file repository '/Users/leopold/.aiida/repository-test_aiida/'?
WARNING: All data will be lost. [y/N]: y
Deleting directory '/Users/leopold/.aiida/repository-test_aiida/'.
Delete configuration for profile 'test_aiida'?
WARNING: This permanently removes the profile from the list of AiiDA profiles. [y/N]: y
Deleting configuration for profile 'test_aiida'.

ltalirz and others added 5 commits July 28, 2017 18:11
verdi profile delete now checks whether the database of the profile
actually exists and whether the database user is used by any other
existing profiles.

further adjustments to coding style following Sebastiaans suggestions.
Copy link
Contributor

@DropD DropD left a comment

Choose a reason for hiding this comment

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

Looks good aside of a few minor things (see comments).
For the future: profile delete should maybe be a separate PR

import os.path
from urlparse import urlparse

#TODO: the functions used below should be moved outside Quicksetup
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please make this into an issue instead of a TODO comment? You can assign it to me if you don't want to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also include in the issue that there are no tests for these functions as of yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

try:
profile = profiles[profile_to_delete]
except KeyError:
raise ValueError('Profile "{}" does not exist'.format(profile_to_delete))
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be more user friendly to either:

  • check for inexistent profiles before starting to delete (and abort)
  • give user an option to skip inexistent profiles (either cli or interactively or both)
  • simply treat deleting a nonexistent profile as a noop (and maybe issue a warning)

Copy link
Member Author

Choose a reason for hiding this comment

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

right, I'm going for the 3rd option

db = profile.get('AIIDADB_NAME', '')
if not q._db_exists(db, pg_execute, **dbinfo):
print("Associated database '{}' does not exist.".format(db))
elif click.confirm("Delete associated database '{}'?\n" \
Copy link
Contributor

Choose a reason for hiding this comment

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

For usage with scripts (for example to automatically reset a testing environment), it would be helpful to have a non-interactive way to confirm deleting everything, say a --force option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely agree, I am going with '--yes' (in analogy with apt-get).
For the moment, I check by hand whether '--yes' is in the argument list as this is the way it is done with '--color' in another function in the same file.
To be replaced by more robust code in the future.

elif users.count(user) > 1:
print("Associated database user '{}' is used by other profiles "\
"and will not be deleted.".format(user))
elif click.confirm("Delete database user '{}'?".format(user)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same consideration as above (--force) applies

elif not os.path.isdir(repo_path):
print("Associated file repository '{}' is not a directory."\
.format(repo_path))
elif click.confirm("Delete associated file repository '{}'?\n" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Another case for --force

"WARNING: All data will be lost.".format(repo_path)):
print("Deleting directory '{}'.".format(repo_path))
import shutil
shutil.rmtree(repo_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would make sense to already delegate the task of deleting to another function in a more obvious place which will do case handling for file system vs. object store / other future options - repositories.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the moment, I've created an issue.
#695

Will use more general functionality once it becomes available.

import shutil
shutil.rmtree(repo_path)

if click.confirm("Delete configuration for profile '{}'?\n" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Another --force case

set_default_profile(process, profile_name, force_rewrite=True)

#TODO (ltalirz) db-related functions should be moved to the base level of verdilib
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an issue for this (instead of a comment) and assign it to me unless you are interested in fixing it yourself

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

for running in interactive mode
@ltalirz
Copy link
Member Author

ltalirz commented Sep 12, 2017

@DropD Thanks for the constructive suggestions, let me know what you think

as pointed out by Rico, --force conveys that the questions
are answered in a way such as to lead to removal of the profile
@DropD DropD merged commit 6527b8a into aiidateam:develop Sep 12, 2017
@szoupanos
Copy link
Contributor

One question/comment on this.

Since we going to click for modularity and easy testing of the CLI, wouldn't it be good to start adding some tests? Especially for new commands that are developed with click?

This will make our lives easier in the future.

@ltalirz ltalirz deleted the fix_XXX_verdi_quicksetup branch October 31, 2017 18:11
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.

5 participants