-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Async api #60
base: main
Are you sure you want to change the base?
feat: Async api #60
Conversation
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
if self.task_status in [TaskStatus.SUCCESS, TaskStatus.FAILURE]: | ||
return True | ||
return False |
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 self.task_status in [TaskStatus.SUCCESS, TaskStatus.FAILURE]: | |
return True | |
return False | |
return self.task_status in [TaskStatus.SUCCESS, TaskStatus.FAILURE] |
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 think ruff's SIM
would suggest this fix
task_id: str = await self.orchestrator.task_queue.get() | ||
self.orchestrator.queue_list.remove(task_id) | ||
|
||
if task_id not in self.orchestrator.tasks: | ||
raise RuntimeError(f"Task {task_id} not found.") | ||
task = self.orchestrator.tasks[task_id] |
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.
shouldnt this be a part of AsyncLocalOrchestrator
API? too easy to make a mistake with 3 separate properties
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
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.
✨ Beautiful! Only small comments below.
self.task_queue = asyncio.Queue() | ||
self.tasks: Dict[str, Task] = {} | ||
self.queue_list: List[str] = [] |
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.
Maybe one could encapsulate these three members in a class that derives from asyncio.Queue to make it cleaner and less error prone to use.
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 think this and other refactoring will become more obvious once we implement the other compute engines (redis, etc)
The idea is also to have functionalities in some base class or objects.
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Signed-off-by: Michele Dolfi <[email protected]>
Introduce the Async compute API
Still TODO
return_as_file
or multiple inputs)Issue resolved by this Pull Request:
Resolves #51