Skip to content
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

[task scheduling] scope TaskRunner to TaskServer #11806

Merged
merged 20 commits into from
Feb 2, 2024

Conversation

zzstoatzz
Copy link
Collaborator

@zzstoatzz zzstoatzz commented Feb 1, 2024

this PR:

  • allows specification of a task_runner via serve (to be used for all task runs picked up by that TaskServer)
  • refactors TaskServer context management, in particular to make sure we only start / stop a TaskRunner with the lifetime of the TaskServer (i.e. not once per task submission)
  • fixes a sneaky sync/async bug where sync tasks were awaitable when called in an async context

stacked on top of #11805, so will not merge until that is

zzstoatzz and others added 5 commits February 1, 2024 15:48
The goal here is for each subscriber to be able to filter to a subset of the
tasks they are interested in (i.e. which tasks a task server is serving).  This
required routing task runs into queues and monitoring multiple queues for
tasks.  This was a good opportunity to refactor some of the mechanics here to
capture the idea that a `TaskQueue` is really two queues (a "regular" queue and
a high-priority retry queue).  This led me down a path of tidying up the
implementation to remove globals and encapsulate more of the behavior.
Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for prefect-docs-preview ready!

Name Link
🔨 Latest commit 9eab2be
🔍 Latest deploy log https://app.netlify.com/sites/prefect-docs-preview/deploys/65bd170be02ae4000890697a
😎 Deploy Preview https://deploy-preview-11806--prefect-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@zzstoatzz zzstoatzz self-assigned this Feb 1, 2024
@zzstoatzz zzstoatzz added the DONT MERGE This PR shouldn't be merged (yet) label Feb 2, 2024
@zzstoatzz zzstoatzz removed the DONT MERGE This PR shouldn't be merged (yet) label Feb 2, 2024
create_autonomous_task_run
)
else:
return from_sync.wait_for_call_in_loop_thread(
Copy link
Collaborator Author

@zzstoatzz zzstoatzz Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part handles the sync within async correctly where sync_compatible does not

@zzstoatzz zzstoatzz marked this pull request as ready for review February 2, 2024 15:34
@zzstoatzz zzstoatzz requested a review from a team as a code owner February 2, 2024 15:34
Copy link
Collaborator

@chrisguidry chrisguidry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice!

@@ -234,9 +233,9 @@ class EngineContext(RunContext):
flow: Optional["Flow"] = None
flow_run: Optional[FlowRun] = None
autonomous_task_run: Optional[TaskRun] = None
task_runner: BaseTaskRunner
task_runner: Any
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really Any or is it Optional[BaseTaskRunner]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was slightly over-complicating this in the case where we want to re-use a task runner, we can just pass the task runner instance along instead of entering a null context and then the typing stays the same

updated in 1a5a047

- tags: A list of tags to apply to the task server. Defaults to `["autonomous"]`.
- task_runner: The task runner to use for executing the tasks. Defaults to
`ConcurrentTaskRunner`.
- tags: A list of tags to add to task runs submitted by the task server.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still doing the tags thing? The TaskServer doesn't submit tasks anymore, right?

Copy link
Collaborator Author

@zzstoatzz zzstoatzz Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh i see what you mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm'd in 9eab2be

@zzstoatzz zzstoatzz merged commit 9dfbef4 into main Feb 2, 2024
59 of 60 checks passed
@zzstoatzz zzstoatzz deleted the task-server-task-runner branch February 2, 2024 16:42
@serinamarie serinamarie added the enhancement An improvement of an existing feature label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants