Refactor Computer and add --format option to verdi computer show
#3616
Conversation
The `aiida.orm.computers.Computer` front-end class had a mixture of properties and methods to get and set various properties. Which one was available depended on how that property was stored in the backend. Those that are stored as database columns were exposed through property getter and setters, whereas properties that are part of the `metadata` JSONB column had individual methods. Besides being inconsistent, having explicit methods for each property, of which there are already quite a few and which might increase in the future, is inconvenient. It also makes it more difficult to programmatically get all the properties that "define" a computer. By defining all properties in a mapping class attribute, one can define all setters and getters dynamically.
This allows to, in addition to the default table display, dump the contents of a computer in yaml format which can in turn be used for the `--config` option of `verdi computer setup`. This makes the sharing of a particular computer that has been setup very easy. Sharing yaml config files is more apt than AiiDA export archives which was the only possible way of sharing `Computer` instances to date.
There was a problem hiding this comment.
Thanks @sphuber - I fully agree with the direction of the PR.
I started looking through dir(Computer()) and I have some questions:
- do we want to expose the
PROPERTY_...variables at the class level for autocompletion? - what decides whether a certain quantity gets its own
@propertyor whether it needs to go viaget_property/set_propertymethods? Perhaps we should add the list to the commit message:
Accessible via property: description, hostname, id, label, pk, scheduler_type, transport_type - The naming of "property" here vs "attributes/extras" in nodes. I guess semantically they are extras in the sense that one can simply change them. I was surprised to find that you can do that via the python API, while the CLI clearly suggests to clone the computer if you need to. I think this is problematic from a provenance point of view.
Below the output of dir(Computer()), minus the methods starting with double underscores.
I've marked deprecated methods with an X.
['Collection',
'PROPERTY_APPEND_TEXT',
'PROPERTY_MINIMUM_SCHEDULER_POLL_INTERVAL',
'PROPERTY_MPI_PROCES_PER_MACHINE',
'PROPERTY_MPI_RUN_COMMAND',
'PROPERTY_PREPEND_TEXT',
'PROPERTY_SHEBANG',
'PROPERTY_WORK_DIR',
'_backend_entity',
'_called',
'_logger',
'_objects',
'_property_mapping',
'backend',
'backend_entity',
'configure',
'copy',
'delete_property',
'description',
'from_backend_entity',
'full_text_info', X
'get',
'get_append_text', X
'get_authinfo',
'get_configuration',
'get_default_mpiprocs_per_machine', X
'get_description', X
'get_hostname', X
'get_metadata',
'get_minimum_job_poll_interval', X
'get_mpirun_command', X
'get_name', X
'get_prepend_text', X
'get_properties',
'get_property',
'get_scheduler',
'get_scheduler_type', X
'get_schema', X
'get_shebang', X
'get_transport',
'get_transport_class',
'get_transport_type', X
'get_workdir',X
'hostname',
'id',
'init_from_backend',
'initialize',
'is_stored',
'is_user_configured',
'is_user_enabled',
'label',
'logger',
'name', X
'objects',
'pk',
'scheduler_type',
'set_append_text', X
'set_default_mpiprocs_per_machine', X
'set_description', X
'set_hostname', X
'set_metadata',
'set_minimum_job_poll_interval', X
'set_mpirun_command', X
'set_name', X
'set_prepend_text', X
'set_property',
'set_scheduler_type', X
'set_shebang', X
'set_transport_type', X
'set_workdir', X
'store',
'transport_type',
'uuid']
| @@ -245,7 +245,7 @@ def code_list(computer, input_plugin, all_entries, all_users, show_owner): | |||
|
|
|||
| qb_computer_filters = dict() | |||
| if computer is not None: | |||
| qb_computer_filters['name'] = computer.name | |||
| qb_computer_filters['name'] = computer.lbael | |||
There was a problem hiding this comment.
| qb_computer_filters['name'] = computer.lbael | |
| qb_computer_filters['name'] = computer.label |
There was a problem hiding this comment.
not sure whether this is also in other places (I guess not?) - a quick search should show
| @@ -130,21 +128,13 @@ def get_workdir(self): | |||
| try: | |||
| return self.get_metadata()[self.PROPERTY_WORKDIR] | |||
| except KeyError: | |||
| return self.computer.get_workdir() | |||
| return self.computer.get_property('work_dir') | |||
There was a problem hiding this comment.
The property seems to be called workdir - at least on profile (on your branch) I get:
In [1]: c=load_computer(1)
In [2]: c.get_metadata()
Out[2]:
{'shebang': '#!/bin/bash',
'workdir': '/scratch/{username}/aiida_run/',
'append_text': ' ',
'prepend_text': ' ',
'mpirun_command': ['srun', '-n', '{tot_num_mpiprocs}'],
'default_mpiprocs_per_machine': 28}
would this be broken now?
|
This was my review of the PR (but it does contain a couple of questions). |
|
Yes yes, I was just checking if after @sphuber addressed them it could be merged or further full reviewing was needed. |
|
This needs quite a bit more discussion. The goal was to homogenize the interface of the |
|
Would it help to bring it for discussion during the next fortnightly meeting? If it needs more than that and we are not sure when we'll be able to deal with it maybe we should close the PR? |
|
Since I think this requires quite a bit more discussion and work, I will close this for now |
|
Just one comment in case someone wants to pick this up again: I think fixing #3521 (i.e. adding the yaml export) is quite straightforward. |
An ad-hoc implementation would indeed be straightforward, but this would most likely hard code which properties need to be dumped, how to fetch them from a computer instance and to what keys they should be written. The whole reason why my attempt became so much more involved was because I was exactly trying to find a way to avoid this, for which a significant redesign of these ORM classes is necessary |
Fixes #2135 and fixes #3521