-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
tests/test_vertext_worker.py
Outdated
@@ -0,0 +1,123 @@ | |||
import uuid |
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 need to add the documentation docstring + changelog |
CustomJob
workerCustomJob
worker
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 did a good bit of testing with this this morning and it was working great! Nice work!!
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.
Thanks for creating this so quickly! I have a few comments on structure which may have an impact on implementation. Once we've sorted them out then I can make another detailed pass. Let me know if you have any questions!
f82996e
to
effe02e
Compare
"""Returns a base job body to use for job spec validation. | ||
Note that the values are stubbed and are not used for the actual job.""" | ||
return { | ||
"maximum_run_time_hours": "1", |
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.
im just validating against the presence of keys, so not going to validate against the provided values (since they generally come from a set of acceptable values)
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.
Final round of comments after a more thorough review. Once these are taken care of, this should be good to go.
I noticed that there's no kill_infrastructure
command on this worker needed to support flow run cancellation. If you'd rather leave that out of this PR, let me know, and I can create an issue to track adding it later.
@desertaxle doh - let me add that, but in a follow up PR bc this one is getting beefy with comments. i'll create an issue for it so i dont forget |
_documentation_url = "https://prefecthq.github.io/prefect-gcp/worker/" | ||
_logo_url = "https://images.ctfassets.net/gm98wzqotmnx/4SpnOBvMYkHp6z939MDKP6/549a91bc1ce9afd4fb12c68db7b68106/social-icon-google-cloud-1200-630.png?h=250" # noqa | ||
_documentation_url = "https://prefecthq.github.io/prefect-gcp/cloud_run_worker/" | ||
_logo_url = "https://cdn.sanity.io/images/3ugk85nk/production/10424e311932e31c477ac2b9ef3d53cefbaad708-250x250.png" # noqa |
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.
this pr splits up the worker docs, so this page url needed updating as well
i also swapped the logo urls for all the links, lemme know if i went too far
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.
That's awesome! Thanks!
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.
LGTM! 🚀
This PR adds a
Vertex AI
Custom Worker type, which is a port over from the existing InfrastructureVertexAICustomTrainingJob
blockCloses PrefectHQ/prefect#10478
Example
Screenshots
Docs
Checklist
pre-commit
checks.pre-commit install && pre-commit run --all
locally for formatting and linting.mkdocs serve
view documentation locally.