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

πŸ› Setting user (Email) to none is possible but fails quickly #6030

Closed
mbercx opened this issue May 18, 2023 · 8 comments Β· Fixed by #6033
Closed

πŸ› Setting user (Email) to none is possible but fails quickly #6030

mbercx opened this issue May 18, 2023 · 8 comments Β· Fixed by #6033

Comments

@mbercx
Copy link
Member

mbercx commented May 18, 2023

Describe the bug

While running verdi profile setup, it's possible not to set the user (which is actually an email, also slightly strange):

❯ verdi setup
Report: enter ? for help.
Report: enter ! to ignore the default and set no value.
Profile name: dev      
Email Address (for sharing data) [()]: 
Error: Please enter a valid email.
Email Address (for sharing data) [()]: !
First name [()]: !
Last name [()]: !
Institution [()]: !
Database engine (postgresql_psycopg2) [postgresql_psycopg2]: 
Database backend (core.psql_dos) [core.psql_dos]: 
Database host [localhost]: 
Database port [5432]: 
Database name: core-dev
Database username: mbercx
Database password: 
Broker protocol (amqp, amqps) [amqp]: 
Broker username [guest]: 
Broker password [guest]: 
Broker host [127.0.0.1]: 
Broker port [5672]: 
Broker virtual host name []:  
Repository directory [/Users/mbercx/project/core/.aiida/repository/dev]: 
Success: created new profile `dev`.
Report: initialising the profile storage.
Report: initialising empty storage schema
Report: Migrating to the head of the main branch
Success: storage initialisation completed.

All good, ready to go I'd say πŸš€. Alas, the party ends quite quickly:

In [1]: Int(1)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[1], line 1
----> 1 Int(1)

File ~/project/core/code/aiida-core/aiida/orm/nodes/data/base.py:33, in BaseType.__init__(self, value, **kwargs)
     30 except AttributeError:
     31     raise RuntimeError('Derived class must define the `_type` class member')
---> 33 super().__init__(**kwargs)
     35 self.value = value or self._type()

File ~/project/core/code/aiida-core/aiida/orm/nodes/data/data.py:50, in Data.__init__(self, source, *args, **kwargs)
     48 def __init__(self, *args, source=None, **kwargs):
     49     """Construct a new instance, setting the ``source`` attribute if provided as a keyword argument."""
---> 50     super().__init__(*args, **kwargs)
     51     if source is not None:
     52         self.source = source

File ~/project/core/code/aiida-core/aiida/orm/nodes/node.py:189, in Node.__init__(self, backend, user, computer, **kwargs)
    186 user = user if user else backend.default_user
    188 if user is None:
--> 189     raise ValueError('the user cannot be None')
    191 backend_entity = backend.nodes.create(
    192     node_type=self.class_node_type, user=user.backend_entity, computer=backend_computer, **kwargs
    193 )
    194 super().__init__(backend_entity)

ValueError: the user cannot be None

Expected behavior

If the user is necessary to run with the profile it should not be possible to not specify it during setup. That said, why is the user necessary just to use AiiDA? I understand that e.g. only the groups owned by the current user are shown when doing e.g. verdi group list, but I would argue this actually detracts from the UX (I download archive. I import archive. I do verdi group list and see no groups).

In fact, do we really have to ask people's names too? And their Institution even? I think there are all unnecessary and hence can be optional but removed from the setup.

Your environment

  • Operating system [e.g. Linux]: MacOS
  • Python version [e.g. 3.7.1]: 3.9.16
  • aiida-core version [e.g. 1.2.1]: main branch

Additional context

Also I could not find a specific issue with a cursory search, this is heavily related to all issues concerning AiiDA uptake, i.e. getting beginners up and running as fast as possible with AiiDA.

@mbercx
Copy link
Member Author

mbercx commented May 18, 2023

Also not easy to fix this? I think I have to delete the profile and make it again.

@mbercx
Copy link
Member Author

mbercx commented May 18, 2023

Interestingly, just trying to press enter when the already stored None default is there does fail:

❯ verdi profile delete dev
Warning: deleting profile `dev` excluding: database user.
Warning: this operation cannot be undone, are you sure you want to continue? [y/N]: y
Success: profile `dev` was deleted excluding: database user..
❯ verdi setup
Report: enter ? for help.
Report: enter ! to ignore the default and set no value.
Profile name: dev
Email Address (for sharing data) [None]: 
Error: Please enter a valid email.

@mbercx
Copy link
Member Author

mbercx commented May 18, 2023

Also noticed that my database was silently deleted when deleting the profile. This should not be the default.

❯ verdi profile delete --help
Usage: verdi profile delete [OPTIONS] PROFILES...

  Delete one or more profiles.

  The PROFILES argument takes one or multiple profile names that will be
  deleted. Deletion here means that the profile will be removed including its
  file repository and database. The various options can be used to control
  which parts of the profile are deleted.

Options:
  -f, --force                     to skip questions and warnings about loss of
                                  data
  --include-config / --skip-config
                                  Include deletion of entry in configuration
                                  file.  [default: include-config]
  --include-db / --skip-db        Include deletion of associated database.
                                  [default: include-db]

@sphuber
Copy link
Contributor

sphuber commented May 21, 2023

That said, why is the user necessary just to use AiiDA?

Because in the current database schema, most tables, such as DbNode and DbGroup, define a required foreign key on DbUser. In other words, each Node, each Group etc. have to have a user set, or it cannot be stored.

Now one can of course question this decision, but that has been there since the very first version of AiiDA, and changing it would require database migrations and lots of changes in a lot code to deal with the assumption that a user will always be set.

On to the actual bug: I think this may be a problem in the (quite complex) logic of InteractiveOptions. The user option is defined as:

SETUP_USER_EMAIL = options.USER_EMAIL.clone(
    prompt='Email Address (for sharing data)',
    default=get_config_option('autofill.user.email'),
    required_fn=lambda x: get_config_option('autofill.user.email') is None,
    required=True,
    cls=options.interactive.InteractiveOption
)

So as you can see, it is required, which is why passing None explicitly raises, as it should. However, I think the bug is hiding due to required_fn also being defined. I think when you press !, the code whill override the required settings, because required_fn returns False in your case, since get_config_option('autofill.user.email') returns a value, the username you used the first time you ran the setup.

@sphuber
Copy link
Contributor

sphuber commented May 21, 2023

Confirmed. The problem is that required_fn returns a value and so it overrides the option being required, but instead of returning the default, None is returned due to the ! override character being specified. I will submit a fix.

@mbercx
Copy link
Member Author

mbercx commented May 21, 2023

Now one can of course question this decision, but that has been there since the very first version of AiiDA, and changing it would require database migrations and lots of changes in a lot code to deal with the assumption that a user will always be set.

Yes, I can see how the user, i.e. the requested email, is an integral part of the code base. That said:

  1. We can maybe still remove the requests for "First Name", "Last name" and "Institution"?
  2. We could just have a default username ([email protected] for example). Not saying we need to, but it's a possibility that would work, correct?

Now, I don't see these questions as huge barriers for beginning users, obviously. But I do vaguely remember being annoyed at having to go through these again and again when I was first trying to set up a profile (without YAML files, wild times). I think there is an argument to be made for streamlining this even more and allowing these user details to be configured afterwards using verdi config, so they can be set at different levels.

@sphuber
Copy link
Contributor

sphuber commented May 22, 2023

We can maybe still remove the requests for "First Name", "Last name" and "Institution"?

This falls in the same category of complaints you had about the number of prompts when configuring a computer with the core.ssh transport. It would be great if you can open an issue that starts collecting the options for these various commands that should not be prompted for, if they have a sensible default that a first user typically won't want to change. We can then think of how to implement this, because it would require some additional functionality to "flag" these options that should not be prompted for.

We could just have a default username ([email protected] for example). Not saying we need to, but it's a possibility that would work, correct?

This would work indeed, but it would still be prompted for, until we implement the feature mentioned above. So the user would still have to press enter, but most likely is going to be either more confused or will end up typing their own email anyway. So not sure that would help much.

@mbercx
Copy link
Member Author

mbercx commented May 22, 2023

It would be great if you can open an issue that starts collecting the options for these various commands that should not be prompted for, if they have a sensible default that a first user typically won't want to change.

Fair point, I have already tried to do something similar for verdi computer setup in #4981, but had to look a bit before I found it and it would be good to have a comprehensive list/overview we can discuss. Will try to work on this later this week πŸ‘ (Perhaps something that could be the topic of the coding week)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants