-
Notifications
You must be signed in to change notification settings - Fork 94
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
Break up task edge registration #238
Conversation
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.
Good sleuthing!
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.
Great work digging into the subtleties here both client and server side!
Co-authored-by: Michael Adkins <[email protected]>
All feedback has been addressed, albeit likely imperfectly Would be good to get another set of eyes on the exception handling here https://github.com/PrefectHQ/server/pull/238/files#diff-23db4a75ea24edcd454c5f66ca3e1bf042862bc3713786a8924acd5e06cc256dR320. Slightly confused on what the best pattern for this would be, but logging the error and raising a custom exception from the original seemed somewhat reasonable |
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.
Some really minor comments, overall this is looking good to me
Summary
This PR batches
Task
andEdge
creation when creating aFlow
.Importance
For very large flows, we're hitting a limit on Postgres query size when trying to create the full Flow model. See this Slack thread for a full discussion https://prefect-community.slack.com/archives/C014Z8DPDSR/p1617128789186300.
I think eventually we'll want to do this batching on the client side, but this PR along with an update to speed up flow validation will allow registration of fairly large flows for the time being.
Checklist
This PR:
changes/
directory (if appropriate)