feat: Add Async Job Scheduler #3970
Conversation
meta-cla
bot
added
the
CLA Signed
slekkala1
force-pushed
the
job-scheduler
branch
from
9e6d7b8 to
e3d9051
Compare
slekkala1
requested review from
ashwinb,
bbrowning,
ehhuang,
franciscojavierarceo,
hardikjshah,
leseb,
mattf,
raghotham,
reluctantfuturist,
terrytangyuan and
yanxi0830
as code owners
| from typing import Protocol | ||
|
|
||
|
|
||
| class JobStatus(StrEnum): |
There was a problem hiding this comment.
statuses for batch apis in OpenAI are:
status: Literal[
"validating", "failed", "in_progress", "finalizing", "completed", "expired", "cancelling", "cancelled"
]it could be rational for us to follow their statuses with the additional PENDING state (since I assume they lump that into in_progress).
There was a problem hiding this comment.
Updated, thanks for the pointer!
|
@slekkala1 it would be good to file an Issue for this work if one doesn't exist describing why this is being undertaken and perhaps referencing the older discussions / issues also which I had shared with you. |
|
|
||
| from .scheduler import CelerySchedulerImpl | ||
|
|
||
| __all__ = ["CelerySchedulerImpl"] |
There was a problem hiding this comment.
this looks almost like you're constructing an API for /jobs inside of provider/utils. Should this instead be an API?
There was a problem hiding this comment.
@cdoern that is quite intended since we do not want to expose them as public APIs to the end user. these are "stack internal" APIs. It would be good to have an "infrastructure" for these APIs (particularly if want external providers to target them maybe?) but we don't have that right now. Our KVStore / SQLStore APIs are similar in spirit.
There was a problem hiding this comment.
@cdoern I do think it is a good idea to create llama_stack_api.internal and situate these APIs there. Could be a good follow-up. Maybe I will try to get the kvstore / sqlstore ones there once your PR lands.
| CANCELLED = "cancelled" | ||
|
|
||
|
|
||
| class Scheduler(Protocol): |
There was a problem hiding this comment.
same comment as above, this is like a shadow API inside of providers/utils. I think it'd make sense to expose this at a higher level. WDYT?
There was a problem hiding this comment.
@cdoern yes these are the internal apis same as KVstore/SQL store APIs (as Ashwin mentions above)
Filed a new issue #4179 linked to the old issue inside it. |
slekkala1
force-pushed
the
job-scheduler
branch
from
e0c3381 to
55a01e1
Compare
There was a problem hiding this comment.
since this PR adds types and an implementation, should we implement some of these methods? otherwise we are introducing an API structure without validation its the right shape for any of the providers.
alternatively, lets just drop the celery stub until we actually go to implement it.
There was a problem hiding this comment.
+1.
We can also have a full implementation here and split it up into two PRs.
There was a problem hiding this comment.
I vote to split into API and Implementation, 2 PRs. Thanks!
What does this PR do?
Linked issue #4179
Current PR Scope (Stubs + API)
Future PR Scope (Implementation)
Test Plan
UTs