Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

WorkerV2 - Cloud Run V2 API #220

Conversation

japerry911
Copy link
Contributor

@japerry911 japerry911 commented Oct 22, 2023

New: Add support for Cloud Run API v2 (Worker)

Still ToDo:

Overview

How to Test Locally:

Screenshot 2023-10-26 at 11 03 14 PM

Example

  • Anonymous Block example:
CloudRunJobV2(
    credentials=gcp_credentials_block,  # GCPCredentials Block
    image=(
        f"us-central1-docker.pkg.dev/{variables.GCP_PROJECT_ID}/prefect-flows/flows:"
        f"{variables.IMAGE_TAG}"
    ),
    region="us-central1",
    cpu=1,
    memory=512,
    memory_unit="Mi",
    env=variables.GCP_BLOCK_ENV,  # environment variables object
    keep_job=False,
    timeout=86400,  # 24 hours
    launch_stage="BETA",
    max_retries=1,
)

Screenshots

  • Starting Deployment/Flow
    Screenshot 2023-10-26 at 11 00 38 PM

  • Starting in Cloud Run Jobs via Cloud Run API V2
    Screenshot 2023-10-26 at 11 00 53 PM

  • Full 1 day timeout being used in example
    Screenshot 2023-10-26 at 11 01 21 PM

  • Cancelling Example (currently delete all executions by hand instead of using Force parameter until GCP fixes that error in above issue)
    Screenshot 2023-10-26 at 11 02 11 PM
    Screenshot 2023-10-26 at 11 02 15 PM

Thank you, let me know of any/all questions/comments/feedback, happy to have worked on this.

Checklist

  • References any related issue by including "Closes #" or "Closes ".
    • If no issue exists and your change is not a small fix, please create an issue first.
  • Includes tests or only affects documentation.
  • Passes pre-commit checks.
    • Run pre-commit install && pre-commit run --all locally for formatting and linting.
  • Includes screenshots of documentation updates.
    • Run mkdocs serve view documentation locally.
  • Summarizes PR's changes in CHANGELOG.md

@japerry911 japerry911 marked this pull request as ready for review October 27, 2023 05:08
@japerry911 japerry911 requested a review from a team as a code owner October 27, 2023 05:08
Copy link
Contributor

@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.

hey @japerry911 - thank you so much for the work here!

I've done a quick run through the v2 worker in particular and have a few comments / questions 🙂

  • this is looking really good so far! we really appreciate thorough PRs like this - especially a more complex implementation like a worker 💙
  • would you be willing to copy the infra block implementation to a separate PR? i think it would be good for future reference if they were separate - plus we'll have to think a little about the ambiguity of Cloud Run v2 vs Cloud Run v2 Worker in the docs / collection registry. a related nit, could we update this PR name to be "{verb} {feature}" like "Add v2 Cloud Run API worker" (if that sounds good to you)?
  • i am wondering if there are some places we are using Optional where its simply a field having a default value, whose value should generally not be None (left comment on at least one of them)

I will review the tests and the rest as soon as I can, and take it for a spin :) ! feel free to reach out with any questions

prefect_gcp/workers/cloud_run_v2.py Outdated Show resolved Hide resolved
prefect_gcp/workers/cloud_run_v2.py Show resolved Hide resolved
prefect_gcp/workers/cloud_run_v2.py Outdated Show resolved Hide resolved
prefect_gcp/workers/cloud_run_v2.py Outdated Show resolved Hide resolved
@zzstoatzz zzstoatzz added the enhancement New feature or request label Nov 2, 2023
@japerry911
Copy link
Contributor Author

hey @japerry911 - thank you so much for the work here!

I've done a quick run through the v2 worker in particular and have a few comments / questions 🙂

  • this is looking really good so far! we really appreciate thorough PRs like this - especially a more complex implementation like a worker 💙
  • would you be willing to copy the infra block implementation to a separate PR? i think it would be good for future reference if they were separate - plus we'll have to think a little about the ambiguity of Cloud Run v2 vs Cloud Run v2 Worker in the docs / collection registry. a related nit, could we update this PR name to be "{verb} {feature}" like "Add v2 Cloud Run API worker" (if that sounds good to you)?
  • i am wondering if there are some places we are using Optional where its simply a field having a default value, whose value should generally not be None (left comment on at least one of them)

I will review the tests and the rest as soon as I can, and take it for a spin :) ! feel free to reach out with any questions

@zzstoatzz All this sounds great, thank you for diving in!

Good idea, I'll break it out into two branches over the next few days (can probably get it done by Friday morning). Should I open PR for just Cloud Run v2 Block first, then the worker after that merges, or what's the best way to go about it?

I left some comments/questions on a few of the modifications. Lemme know what you think, and I will make all of those updates. And lastly, I'll take a deeper look at "i am wondering if there are some places we are using Optional where its simply a field having a default value, whose value should generally not be None (left comment on at least one of them)".

Thanks again for review @zzstoatzz ! Looking forward to getting these PRs into the repo.

@zzstoatzz
Copy link
Contributor

zzstoatzz commented Nov 2, 2023

Good idea, I'll break it out into two branches over the next few days (can probably get it done by Friday morning). Should I open PR for just Cloud Run v2 Block first, then the worker after that merges, or what's the best way to go about it?

i think the worker is a good place to start if that sounds alright, since the worker implementations should be a little more up to date with our best practices in core and could inspire some updates to the infra block implementation (plus just generally we are more focused on actively maintaining workers nowadays)

thanks for the quick updates :)

@japerry911
Copy link
Contributor Author

Good idea, I'll break it out into two branches over the next few days (can probably get it done by Friday morning). Should I open PR for just Cloud Run v2 Block first, then the worker after that merges, or what's the best way to go about it?

i think the worker is a good place to start if that sounds alright, since the worker implementations should be a little more up to date with our best practices in core and could inspire some updates to the infra block implementation.

thanks for the quick updates :)

The worker uses these imports from the Block though:

from prefect_gcp.cloud_run_v2 import CloudRunJobV2Result, ExecutionV2, JobV2

That's why I went about merging them into 1 PR, since the block would only run with Agent 😿 .

@japerry911
Copy link
Contributor Author

Oh I see, I probably should just build those classes that are imported and keep the others out for a separate PR?

@japerry911 japerry911 changed the title Skylord/new/cloud run v2 and worker v2 active WorkerV2 - Cloud Run V2 API Nov 5, 2023
@japerry911 japerry911 marked this pull request as draft November 5, 2023 15:42
@japerry911 japerry911 marked this pull request as ready for review November 5, 2023 16:24
@japerry911
Copy link
Contributor Author

@zzstoatzz , hey Nate, I made the modifications we spoke about and tested them as well.

Let me know what you think!

Thank you!

@zzstoatzz
Copy link
Contributor

hey @japerry911 - the code is looking great to me 🙂

I will QA this as soon as possible, then come back for a once over :)

@zzstoatzz
Copy link
Contributor

hey @japerry911 - things are looking great!

I am going to discuss with the rest of the team on how exactly we should differentiate (if anything additional) this in the UI and then include this in a metadata scrape - I'll keep this open for the moment while I get that feedback.

I just want to thank you again for this super high quality contribution - thank you so much 💙. Back with you ASAP

@zzstoatzz
Copy link
Contributor

such a great contribution @japerry911 💙 thanks again!!

@zzstoatzz zzstoatzz merged commit 231f68a into PrefectHQ:main Nov 28, 2023
10 checks passed
- AI Platform: aiplatform.md
- Deployment Steps: deployments/steps.md
- Workers:
- Cloud Run: cloud_run_worker.md
- Cloud Run V2: cloud_run_worker_v2.md
Copy link
Contributor

Choose a reason for hiding this comment

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

@zzstoatzz It seems like there was a 404 in loading this that wasn't caught

Copy link
Contributor

Choose a reason for hiding this comment

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

I can work on that @serinamarie - I see another thing in the API docs that needs fixed, too.

Copy link
Contributor

@serinamarie serinamarie Mar 26, 2024

Choose a reason for hiding this comment

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

it was fixed/merged! thanks though

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Cloud Run to use API v2
4 participants