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

Move the functionality of aiida.cmdline.utils.common.get_database_summary to StorageBackend.get_info #5384

Closed
sphuber opened this issue Feb 23, 2022 · 6 comments · Fixed by #5387

Comments

@sphuber
Copy link
Contributor

sphuber commented Feb 23, 2022

The get_database_summary is currently used in verdi storage info and verdi archive inspect. Both of these essentially give an overview of the storage contents. For historical reason, the functionality of getting the info was split in the database and repository, but these are now unified in the StorageBackend. The functionality of get_database_summary should therefore be moved into StorageBackend.get_info.

Note that get_database_summary currently just uses the ORM and so is technically already independent of the StorageBackend, but there might still be an advantage to put it in StorageBackend.get_info anyway, because certain backends may have more efficient ways of retrieving the information other than going through the ORM which might be adding unnecessary overhead.

@chrisjsewell
Copy link
Member

I would be more specific here and say, after #5331,
aiida/cmdline/commands/cmd_storage.py::storage_info and PsqlDosBackend.get_info will essentially be broke,
since they both treat get_info as if it is only returning information about the repository, contrary to the get_info docstring: "Return general information on the storage."

@chrisjsewell chrisjsewell changed the title Move the functionality of aiida.cmdline.utils.common.get_database_summary to StorageBackend.get_info Fix aiida.cmdline.commands.cmd_storage.storage_info and PsqlDosBackend.get_info Feb 23, 2022
@ramirezfranciscof
Copy link
Member

In the end the docstring is correct and the method was adapted, so we can remove the bug tag. But thanks for the "specificity" and the preemptive concern, I guess.

@ramirezfranciscof ramirezfranciscof changed the title Fix aiida.cmdline.commands.cmd_storage.storage_info and PsqlDosBackend.get_info Move the functionality of aiida.cmdline.utils.common.get_database_summary to StorageBackend.get_info Feb 23, 2022
@ramirezfranciscof
Copy link
Member

I'll take care of moving the database info logic in another PR.

(quote from the original PR)

@sphuber anyways this is not a priority right now for 2.0 no? Since this is mostly internal re-organization.

@sphuber
Copy link
Contributor Author

sphuber commented Feb 23, 2022

It is not, but it is easy enough that I will do it soon anyway I think

@sphuber sphuber self-assigned this Feb 23, 2022
@ramirezfranciscof
Copy link
Member

Ok, I still think there might be some merit in discussing how to balance the prospect of having this command output more "unpredictable" information specific to the backend and more "reliable" and backend agnostic statistics.

For example, the current stats that the summary outputs are the aiida nodes, which is a general concept that is independent of the backend, so I don't think it is a bad idea to also get this information in an agnostic way (even though, as you mentioned elsewhere, it might not be the most ideally performant way).

@sphuber
Copy link
Contributor Author

sphuber commented Feb 23, 2022

@ramirezfranciscof makes sense. See #5387 for a suggestion. I think this possibly addresses your concerns.

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.

3 participants