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

Temporarily disable passing worker labels to flow run #16339

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

jakekaplan
Copy link
Contributor

@jakekaplan jakekaplan commented Dec 11, 2024

This PR temporarily removes _give_worker_labels_to_flow_run from the worker's submission loop.

This method uses the CloudClient.update_flow_run_labels. The CloudClient does not have all the special httpx_settings and custom transport setup that we instantiate in the constructor of PrefectClient. This can potentially lead to instability and can lead to httpx.ConnectTimeout errors (and others) when the CloudClient is used in something like the Worker that will need to make a lot of requests. Previously the CloudClient was only for things like 1 of CLI commands.

The _give_worker_labels_to_flow_run method can be added back to the worker's submission loop when update_flow_run_labels is moved over to PrefectClient (which is blocked by https://github.com/PrefectHQ/prefect/pull/16233/files, but to my understanding is going to happen soon). For now it is non critical to have this in place and will hopefully lead to less errors during flow run submission.

Long term we might need consolidate creating our httpx_settings if the CloudClient is going to be used in higher request volume scenarios like orchestration (happy to do this in another pr, but didn't want to jump the gun as it seems the method is going to be moved over to the other client already).

Copy link

codspeed-hq bot commented Dec 11, 2024

CodSpeed Performance Report

Merging #16339 will not alter performance

Comparing remove-ensure-labels (9ad3292) with main (686d4c3)

Summary

✅ 3 untouched benchmarks

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

lgtm!

@zzstoatzz zzstoatzz added the development Tech debt, refactors, CI, tests, and other related work. label Dec 11, 2024
@jakekaplan jakekaplan merged commit 6bfce9e into main Dec 11, 2024
48 checks passed
@jakekaplan jakekaplan deleted the remove-ensure-labels branch December 11, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Tech debt, refactors, CI, tests, and other related work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants