-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update how we start post-processing job #64
Conversation
Minimum allowed coverage is Generated by 🐒 cobertura-action against 720a933 |
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, Natalie! Just added some questions
buildstockbatch/gcp/gcp.py
Outdated
@@ -910,21 +910,23 @@ def start_combine_results_job_on_cloud(self, results_dir, do_timeseries=True): | |||
|
|||
# Create the job | |||
jobs_client = run_v2.JobsClient() | |||
jobs_client.create_job( | |||
op = jobs_client.create_job( | |||
run_v2.CreateJobRequest( |
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.
wondering if we should also change this to request = run_v2.CreateJobRequest(
?
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.
Are you just referring to the variable name? I used op
because create_job
returns an Operation object.
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.
My comment was more of a readability comment. when creating a job with the JobsClient in Google Cloud Run, should you pass the CreateJobRequest object using the request keyword argument?
like
op = jobs_client.create_job(
request = run_v2.CreateJobRequest(
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.
Oh, I see. Updated!
exc_info=True, | ||
) | ||
return | ||
) |
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 makes it much simpler to follow but I feel like we should probably keep the retry and probably change the wait from 1 second to something more robust? Curious to hear your thoughts on removing the retry
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.
The retries were only there to handle the case where the job took a few seconds to be created, causing the first attempt(s) to fail. But now the call to op.result()
above explicitly waits for the job creation to finish. So it's unlikely that just retrying would help anymore.
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, Natalie! Looks good to me!
buildstockbatch/gcp/gcp.py
Outdated
@@ -910,21 +910,23 @@ def start_combine_results_job_on_cloud(self, results_dir, do_timeseries=True): | |||
|
|||
# Create the job | |||
jobs_client = run_v2.JobsClient() | |||
jobs_client.create_job( | |||
op = jobs_client.create_job( | |||
run_v2.CreateJobRequest( |
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.
My comment was more of a readability comment. when creating a job with the JobsClient in Google Cloud Run, should you pass the CreateJobRequest object using the request keyword argument?
like
op = jobs_client.create_job(
request = run_v2.CreateJobRequest(
This should help with the errors we sometimes get when post-processing starts.