-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
WIP Add a way to specify a scheduled time for a task #16
Conversation
from typing import Any, Callable, Dict, Iterator, Optional, Set | ||
|
||
import pendulum |
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.
😗🎶
|
||
if scheduled_time.tzinfo is None: | ||
# Make sure we have an explicit timezone on the schedule time | ||
# We consider a naive datetime to be in the local timezone |
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.
What about forbidding a naive datetime ?
$ python -m this | sed '14q;d'
In the face of ambiguity, refuse the temptation to guess.
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.
That would be ideal, yes.
kwargs=kwargs, | ||
) | ||
|
||
def schedule_at( | ||
self, scheduled_time: datetime, lock: str = None, **kwargs: types.JSONValue |
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.
Just a suggestion: naming the method schedule
, making all parameters kwargs only, naming this parameter "at" ?
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.
The thing is I am starting to think that using **kwargs
to pass the parameters of the underlying task is confusing. It leads to questions like: is at
or lock
keywords of schedule_at()
(this also applies to defer()
) or of the function?
I am trying to figure out a way to make this more explicit. Should defer()
, for instance, only accept the args and kwargs of the decorated function and not its own set of (keyword) arguments? Should some methods have more explicit args
and kwargs
arguments?
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.
Hmm, having explicit args/kwargs is one of the things I dislike most in celery, and often a source of mistake to me. I find this anti-pythonic.
But you're right in that mixing function parameters and defer parameters will lead to frustrating bugs if someone nams their parameter lock, especially if they never use lock elsewhere.
I could see two APIs:
func.defer(lock="bla")(param="something")
or
func.defer(lock="bla").call(param="something")
(this would mean that calling defer() without the second call behind would lead to the task not being launched, which is not ideal either)
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 wonder if using explicit method names and making the default configuration to be specified in the task()
decorator would help here.
For instance, Task.defer
might not need to be able to configure the lock, however a default lock may be passed to task()
. Let's take an example:
import pendulum
from cabbage import TaskManager
manager = TaskManager()
@manager.task(queue="default", lock="my-default-lock")
def add(a, b):
return a + b
add.defer(1, 2) # or add.defer(a=1, b=2)
add.schedule_at(pendulum.now().add(minutes=2), 1, 2)
add.schedule_in(pendulum.parse("PT2M"), a=1, b=2)
However, we should make it possible to override some of the parameters by providing a more feature complete method for special cases:
add.configure(queue="another-queue", lock="another-lock").defer(1, 2)
The names of course are just examples and may change in the final implementation :-)
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.
Scratch the last suggestion, the configure()
method, it wouldn't work. Maybe adding a method to the manager would do the trick.
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.
Why would the configure not work ? I think it's the same as my 2nd suggestion but with other names for methods, and I like your names because "configure" would make it explicit that nothing is called.
A possible implementation:
class Job:
def __init__(self, task, lock, at, ...):
self.everything = everything
def defer(self, **kwargs):
# does what defer does currently
class Task:
def defer(self, **kwargs):
self.configure().defer(**kwargs)
def configure(self, lock=None, at=None):
return Job(task=self, lock=lock, at=None)
# Usage:
@task
def mytask(a, b):
pass
mytask.configure(lock="yay", at=pendulum.now().add(minutes=2)).defer(a=1, b=2)
mytask.defer(a=1, b=2)
If we do that though, I'd rather we don't have a default lock, we don't need one. A task without an explicit lock would simply not be locked.
Also, the Job object might come in handy to gather the information regarding a launch. It's the equivalent "model" of the SQL tasks table
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.
Yes, I experimented a little bit with this and a configure()
method could indeed work. This would make things more explicit if the end user needs more fine-grained configuration when launching a task while keeping things simple by default.
Superseeded by #26 |
This PR adds support for specifying the time a task must be executed.
This is done via a
schedule_at()
method which takes adatetime
object as its first parameter.If the datetime passed to
schedule_at()
is naive it assumes the local timezone.This is still a work in progress because I want to add a way to delay a task by providing a duration. Early feedback is still appreciated though :-)
Note that this PR is based on #15
Closes #8