-
Notifications
You must be signed in to change notification settings - Fork 7
[WIP] Refactor type annotation decorators to use click classes #20
base: master
Are you sure you want to change the base?
Conversation
|
||
def task(name=None, cls=Command, **attrs): |
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.
i'm wondering if there is a way to provide this functionality through the girder_worker.app.app.task
decorator? Otherwise functions will have to look like this:
from girder_worker.app import app
@task()
@task_input('-n', type=types.Integer(min=1), required=True)
@task_output('value', type=types.Integer())
@app.task(...)
def fibonacci(n):
pass
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.
actually i take it back, does this task decorator belong in this PR at all? is it strictly necessary for the item_task functionality to work? See my comment below:
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.
I haven't addressed the integration with girder_worker yet. We could probably combine the two @task
decorators, but for click to work correctly we need a decorator on the top that serves the same purpose of the click.command()
decorator. That's where click actually does all of it's magic.
I think ideally we'd like to avoid introducing a CLI with this PR. That is on the road map, but i'd like to provide a single command that hooks into girder worker's task discovery mechanism - that would allow you list and run all task from the CLI that are installed in your environment. Using something like this import click
from .gwcli import get_extensions, get_extension_tasks
class GWPluginToCommand(click.MultiCommand):
def __init__(self, *args, **kwargs):
super(GWPluginToCommand, self).__init__(*args, **kwargs)
self._tasks = None
def _uniquify_names(self, keys, tasks):
keys = [k.split(".")[-1] for k in keys]
return dict(zip(keys, tasks))
@property
def tasks(self):
if self._tasks is None:
_tasks = []
for ext in get_extensions():
_tasks.extend(get_extension_tasks(ext).items())
self._tasks = self._uniquify_names(*zip(*_tasks))
return self._tasks
def list_commands(self, ctx):
return self.tasks.keys()
def get_command(self, ctx, name):
# get the actual command object Here |
I'm not sure I understand your comment. I had that kind of autogeneration of the multicommand in mind as well as future work. Are you saying this PR shouldn't provide even the single command CLI? |
yeah, so i think |
I thought one of the goals was to avoid requiring celery. My goal here was to lean on click as much as possible. I suspect we need to hash this out in person. |
This commit rewrites how item_tasks decorators is implemented to use click primatives rather than custom logic. The primary advantage is to the new implementation is that the commandline interface comes with the item_task annotations for free. Also, much of the decorating and conversion logic is handled by click classes directly which significantly reduces the number of lines of code. Signed-off-by: Jonathan Beezley <[email protected]>
241c84a
to
120c785
Compare
There is still some work to do on this to handle edge cases related to how data is represented in item_tasks and to add better test coverage. I think the public API here is close enough to the final product that it would be worthwhile for others to look at.
Anyone familiar with click should find the API familiar, for example:
With this you get a CLI:
And you get item_tasks annotations that works with docker:
From the functional API that would be used by the item_task endpoints, it looks very similar to before: