-
Notifications
You must be signed in to change notification settings - Fork 32
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
Dependencies: Make installing plugin packages optional #333
Conversation
34b305e
to
7ada291
Compare
@bosonie what do you think of this proposal? As mentioned in the commit, having to duplicate the requirements in Was thinking about further improvements:
|
986111c
to
907e3d7
Compare
@bosonie I have added a pre-commit hook check for the consistency of the |
34e8858
to
a427012
Compare
@sphuber I had a look at all the commits from my phone. It looks all good to me. The strategy is the same I would implement. |
Thanks for having a look @bosonie . It can wait till next week. It would be great to have another pair of eyes on it and if you can test it a bit. Regarding catching However, this does not catch everything. If the code manually imports a plugin module, e.g., Thinking about it now, it probably still makes sense to do it, even though it won't be perfect and there is a risk for mistakes slipping through the cracks. But in the majority of cases it will give the user actual useful instructions for them to solve the problem themselves and to avoid them coming to the forums with the same questions. I will add another commit on top here later today I think. |
b90a276
to
9a2c9b1
Compare
Ok @bosonie @giovannipizzi , I now also added the last idea of using a custom workflow factory to print This PR is now complete and ready for final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sphuber ! I gave a look to the code and your comments - I think it implements all I think it should. I'm approving it, but since @bosonie said he would have given a careful check tomorrow (and I didn't check the code myself) I suggest we wait to merge until next week, in case he has specific comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sphuber. I've been reviewing the code and I have to report a missing change that is necessary to complete the final feature that raises a better explained MissingEntryPointError
.
When running through the CLI, this is the function called to load the workchains:
def load_workflow_entry_point(workflow: str, plugin_name: str): | |
"""Load the entry point for the given plugin implementation of a certain common workflow. | |
:param workflow: the name of the common workflow. | |
:param plugin_name: name of the plugin implementation. | |
:return: the workchain class of the plugin implementation of the common workflow. | |
""" | |
prefix = f'{PACKAGE_PREFIX}.{workflow}.{plugin_name}' | |
return entry_point.load_entry_point('aiida.workflows', prefix) |
This function must be modified to use the new
aiida_common_workflow.plugins.factories.WorkflowFactory
.
I'll continue a bit tomorrow to test few things but I believe the rest is all good.
9bc4901
to
303f3df
Compare
@sphuber I had a look at the documentation and here you should change the import.
|
303f3df
to
ccf5e9b
Compare
Installing all plugin packages is quite costly and most users are not likely to want to use all codes. Therefore, an optional requirement is created for each code that implements the common workflow interface. To make it easy to still install all plugin packages, the `all_plugins` optional requirement group adds the union of all the plugin optional requirements. The downside is that this requires duplicating the requirements and risks getting out of sync. Built in `[all]` support was proposed in PEP 426 but this was rejected: https://peps.python.org/pep-0426
This provides the `tomllib` package with the standard library which is necessary for an upcoming pre-commit hook.
The `dev/validate_optional_dependencies.py` is added. It validates that the `all_plugins` extras specifies exactly the same dependency requirements that all other extras combined declare as well, except for the `docs`, `pre-commit`, and `tests` extras, which are only used for development. This is to ensure that the `all_plugins` extras provides the exact same dependencies as all the plugin specific extras combined. The script is called through a pre-commit hook.
A new job `tests-minimal-install` is added to the CI and CD workflows. The job installs the package with minimal extra dependencies (just the `tests` extras are installed so that `pytest` is available`. The job then invokes `pytest` with the `-m minimal_install` option. The `minimal_install` marker is added to run only those tests that should run with a minimal install. For now, the minimal install tests simply check that all modules are importable, except for the workflow plugin implementations, and that all CLI commands can be called with the `--help` option.
The `WorkflowFactory` from `aiida-core` is replaced with a custom version in the `aiida_common_workflows.plugins.factories` module. This function will call the factory from `aiida-core` but catch the `MissingEntryPointError` exception. In this case, if the entry point corresponds to a plugin implementation of one of the common workflows the exception is reraised but with a useful message that provides the user with the install command to install the necessary plugin package. While this should catch all cases of users trying to load a workflow for a plugin that is not installed through its entry point, it won't catch import errors that are raised when a module is imported directly from that plugin package. Therefore, these imports should not be placed at the top of modules, but placed inside functions/methods of the implementation as much as possible.
ccf5e9b
to
2bb9b03
Compare
Done. I also fixed the docs in a separate PR, so now all checks are passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks @sphuber
Fixes #233
Fixes #290
Installing all plugin packages is quite costly and most users are not likely to want to use all codes. Therefore, an optional requirement is created for each code that implements the common workflow interface.
To make it easy to still install all plugin packages, the
all_plugins
optional requirement group adds the union of all the plugin optional requirements. The downside is that this requires duplicating the requirements and risks getting out of sync.Built in
[all]
support was proposed in PEP 426 but this was rejected: https://peps.python.org/pep-0426