aiidateam / aiida-common-workflows

A repository for the implementation of common workflow interfaces across materials-science codes and plugins
https://aiida-common-workflows.readthedocs.io
MIT License
52 stars 30 forks source link

Dependencies: Make installing plugin packages optional #333

Closed sphuber closed 7 months ago

sphuber commented 7 months ago

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

sphuber commented 7 months ago

@bosonie what do you think of this proposal? As mentioned in the commit, having to duplicate the requirements in pyproject.toml is not ideal, but as far as I can tell, there is no dynamic way of providing the all_plugins extras, and I think it is necessary to have it because forcing someone to type out all extras if they want support for all plugins is shit UX.

Was thinking about further improvements:

sphuber commented 7 months ago

@bosonie I have added a pre-commit hook check for the consistency of the all_plugins extras. In addition, I have also added tests to make sure the plugin-independent parts of that package can be imported.

bosonie commented 7 months ago

@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. I would have loved to have more time to test it, but I will not be able before Saturday. Up to you if you want to wait (and maybe sort the last point regarding the NonImplementedError) or you prefer to merge and we will sort any possible problem later on.

sphuber commented 7 months ago

@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. I would have loved to have more time to test it, but I will not be able before Saturday. Up to you if you want to wait (and maybe sort the last point regarding the NonImplementedError) or you prefer to merge and we will sort any possible problem later on.

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 ModuleNotFoundErrors: the tricky question is where to put it. I though about subclassing the WorkflowFactory of aiida-core and catching the ImportError. Since we then have the entry point string, which should include the plugin name, we can automatically format the help message to say the should run pip install aiida-common-workflows[plugin_name]. This would require that all code in ACWF consistently uses the custom factory, but that is doable.

However, this does not catch everything. If the code manually imports a plugin module, e.g., from aiida_siesta import workflows, then this cannot be automatically caught. We would have to wrap this in a try/catch ourselves and print the message. This is certainly doable, but it will require some care on our part to make sure that every time a plugin import is added, it is done within this try/catch.

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.

sphuber commented 7 months ago

Ok @bosonie @giovannipizzi , I now also added the last idea of using a custom workflow factory to print pip install instructions when a plugin is requested that is not installed.

This PR is now complete and ready for final review.

bosonie commented 7 months ago

@sphuber I had a look at the documentation and here you should change the import. https://github.com/aiidateam/aiida-common-workflows/blob/303f3df989b9b09b5c7ff63abd1c56250f925f04/docs/source/workflows/base/relax/index.rst?plain=1#L39

sphuber commented 7 months ago

@sphuber I had a look at the documentation and here you should change the import.

https://github.com/aiidateam/aiida-common-workflows/blob/303f3df989b9b09b5c7ff63abd1c56250f925f04/docs/source/workflows/base/relax/index.rst?plain=1#L39

Done. I also fixed the docs in a separate PR, so now all checks are passing.