Make URL Access Pluggable #5323
Conversation
| @@ -0,0 +1,66 @@ | |||
| import importlib | |||
There was a problem hiding this comment.
We probably want to have the copyright header at the top here like we have in the other Toil source files, and also maybe a note (module-level docstring?) about what this file is for.
| def register_plugin( | ||
| plugin_type: PluginType, plugin_name: str, plugin_being_registered: Any | ||
| ) -> None: | ||
| """ | ||
| Adds a plugin to the registry for the given type of plugin. |
There was a problem hiding this comment.
Since the type annotations don't define it, it might be good to note here what the expected plugin values are meant to be for a given plugin type (i.e. functions that return a class implementing blah interface).
It also might be good to explain what plugin_name is used for: for patch systems it is the value the user passes to --batchSystem to actually use it, and for URLs it is the URL scheme value that the plugin gets called to handle. It's not really the "name of the plugin" like the variable name would suggest; you shouldn't pass "My Cool Plugin" or something.
src/toil/lib/plugins.py
Outdated
|
|
||
| def get_plugin(plugin_type: PluginType, plugin_name: str) -> Any: | ||
| """ | ||
| Get a plugin class by name. |
There was a problem hiding this comment.
This sounds a little misleading to me because when I read it I think that:
- The result will be a class, when it's really a function that returns a class when called.
- The argument is the name of the class (like
toil_plugin_url_access_foobar.lib.plugin.MyCoolClass), when it's really a user-facing batch system name or URL scheme.
src/toil/test/lib/url_plugin_test.py
Outdated
| @@ -0,0 +1,108 @@ | |||
| # Copyright (C) 2015-2021 Regents of the University of California | |||
There was a problem hiding this comment.
This should be bumped up to 2025.
src/toil/test/lib/url_plugin_test.py
Outdated
| remove_plugin("url_access", "fake") | ||
| super().tearDown() | ||
|
|
||
| def test_url_exists(self): |
There was a problem hiding this comment.
MyPy is complaining on the CI run that a lot of these test functions are missing a -> None return type annotation.
src/toil/test/lib/url_plugin_test.py
Outdated
| return ["file1.txt", "subdir/"] | ||
|
|
||
| @classmethod | ||
| def _read_from_url(cls, url: ParseResult, writable: io.BytesIO) -> tuple[int, bool]: |
There was a problem hiding this comment.
MyPy complained in CI that the writable needs to be an IO[bytes] like the URLAccess has it, and not an io.BytesIO which MyPy reads as slightly different.
Adding URL plugins by changing the existing plugin system to be more generic. From there, we can apply it to the existing batch system plugins and the new URL access implementation. Documentation will be added when example plugin is added. Fixes #5302.
Changelog Entry
To be copied to the draft changelog by merger:
addBatchSystemFactory,BATCH_SYSTEM_FACTORY_REGISTRY, andBATCH_SYSTEMS. Useadd_batch_system_factoryinstead.toil_url_access_at beginning of python package name. To register a new URL scheme useregister_plugin("url_access", scheme, implementation_type_factory).Reviewer Checklist
issues/XXXX-fix-the-thingin the Toil repo, or from an external repo.camelCasethat want to be insnake_case.docs/running/{cliOptions,cwl,wdl}.rstMerger Checklist