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

Migrating configuration options to AuthInfo and redesigning the interface #3663

Conversation

giovannipizzi
Copy link
Member

I am creating this PR mainly as a discussion place for the design.

This work originally started to fix #3630 (and #3541). However, while working on it, I found a number of design issues that might benefit for a general redesign.

This code shouldn't be merged yet - it starts some moving around, but after further discussions with @muhrin and @sphuber we might want to change properly the current interaction between transport/AuthInfo/scheduler/... classes (and click).

Here are some points after the discussion - once we agree, we can start doing the actual work applying these decisions.

Design issues:

  • AuthInfo it's actually a ComputerConfig object. We don't necessarily need to rename it, but we should think to it that way (all the configuration for a given computer and aiida user pair).
  • It contains a set of properties that can be queried by name. This includes generic things, like the safe open interval and the scheduler poll interval, plus one special property being the auth_params, that are just piped through to the transport to generate a new instance.
  • The transport should not know about the generic properties. Now done in the class by the following code:
    _common_auth_options = [(		
          'safe_interval', {		
              'type': float,		
              'prompt': 'Connection cooldown time (s)',		
              'help': 'Minimum time interval in seconds between consecutive connection openings',		
              'callback': validate_positive_number		
          }		
      )]
    
    Moreover, these properties are sent to the __init__ of the transport, and then just set in an internal variable to then return it back when asked. Since there is no functionality, these variables should never go or be known to the Transport (the transport just knows how to open a transport, the caller is in charge of knowing how often this can be done).
  • These settings above (and the same for the transport-specific ones), anyway, should not be click-specific but we should have our own schema and then a way to convert them to a given UI (e.g. click) - what we need is essentially a type (and a validator), a short and a long help string, and a function to get the default
  • We still need to merge the authinfo-generic keys and the auth_params (transport-specific) settings in the CLI for verdi computer configure xxx
  • we should try to simplify the automatic generation of click commands for verdi computer configure xxx.
  • Moreover, for the entry points, we will remove the entry point aiida.cmdline.computer.configure and generate also those from the aiida.transports entry point (this was already partially happening).
  • We should not have (like I'm originally doing in this WIP code change) the _common_auth_options floating around, but we should have an object that knows about this schema (in a non-click-specific way). Ideally this should be in AuthInfo. The problem is, however, that this means that at import time we need to import aiida.orm (to get the AuthInfo and generate the click decorators to verdi computer configure xxx) and this is slow. We need to decide if we want to solve the speed issue first, and use the AuthInfo object as the main class coordinating all this, or have another class (e.g. aiida.common.computerconfig.ComputerConfig) that does not know about the backend. AuthInfo would just wrap it and provide a "proxy" for the relevant methods, but the logic will be mostly in this internal class, from which the CLI can get the schema information (currently _common_auth_options) without loading aiida.orm.
  • Note: Should this ComputerConfig know about the user and the computer? If so, then it should know about them without needing to go to the orm/DB, otherwise it defeats its purpose. Maybe it just needs to contain the content. However, it needs to know at least the TransportClass it is bound to (maybe in the future also the scheduler?)

Other issues:

  • fix the tests, make sure that everywhere where we were using the safe-interval (or setting it for efficiency in testing) this is now properly honoured
  • update documentation
  • Choose properly the names of options. Since this might be the most important user-facing change, let's pick names we're happy with and:
    • let's provide a data migration
    • let's make sure the old ones still work, deprecated

@giovannipizzi
Copy link
Member Author

Closing for now, while we agree on how to fix this problem

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.

Move the minimum_job_poll_interval from Computer to AuthInfo
1 participant