Skip to content

feat: add job_timeout_ms to job configuration classes #1672

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.

Sign up for GitHub

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

Jump to bottom
Closed
DevStephanie wants to merge 2 commits into from
Closed

feat: add job_timeout_ms to job configuration classes #1672

DevStephanie wants to merge 2 commits into from

Conversation

@DevStephanie
Copy link
Contributor

@DevStephanie DevStephanie commented Oct 4, 2023

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

  • correcting syntax for jobtimeoutms
  • adding test to test_query, based on example comment

@DevStephanie DevStephanie requested review from a team as code owners October 4, 2023 19:02
@DevStephanie DevStephanie requested review from jainsahab and removed request for a team October 4, 2023 19:02
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery API. labels Oct 4, 2023
@tswast tswast changed the base branch from fix/add-jobtimeoutms-to-jobconfig to main October 4, 2023 19:06
@DevStephanie DevStephanie force-pushed the fix/add-jobtimeoutms-to-jobconfig branch from cea062c to 3c9b034 Compare October 4, 2023 19:14
@DevStephanie DevStephanie force-pushed the fix/add-jobtimeoutms-to-jobconfig branch from 3c9b034 to ec045b5 Compare October 4, 2023 19:20
@DevStephanie DevStephanie changed the title Correcting syntax, adding test to test_query.py feat: add job_timeout_ms to job configuration classes Oct 4, 2023
tswast
tswast requested changes Oct 4, 2023
View reviewed changes
google/cloud/bigquery/job/base.py
ValueError: If ``value`` type is invalid.
"""

# None as this is an optional parameter.
Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

It could be None, but it could also be a real integer value. We should return self._properties.get("jobTimeoutMs") (converted to an integer if not None. I believe there are other integer properties that use a helper function for this conversion.)

google/cloud/bigquery/job/base.py

@jobtimeout_ms.setter
def jobtimeout(self, value):
if not isinstance(value, int):
Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

We should also allow None.

google/cloud/bigquery/job/base.py
return None

@jobtimeout_ms.setter
def jobtimeout(self, value):
Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

Make sure this function name matches your @property.

google/cloud/bigquery/model.py
from google.cloud.bigquery import _helpers
from google.cloud.bigquery import standard_sql
from google.cloud.bigquery.encryption_configuration import EncryptionConfiguration
import google.cloud.bigquery.model
Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

We can revert the changes in this file.

tests/unit/job/test_query.py
self.assertEqual(job.ddl_target_table.project, self.PROJECT)

def test_jobtimeout_ms(self):
self.assertIsInstance(job.timeout_ms, value)
Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

Let's set the job configuration timeout ms to something before asserting the value.

Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

Also, move these tests to where we test QueryJobConfig: https://github.com/googleapis/python-bigquery/blob/main/tests/unit/job/test_query_config.py#L20

tests/unit/model/test_model.py
assert object_under_test.labels == {}
object_under_test.labels = {"data_owner": "someteam"}
assert object_under_test.labels == {"data_owner": "someteam"}
assert object_under_test.labels() =={}
Copy link
Contributor

@tswast tswast Oct 4, 2023

Choose a reason for hiding this comment

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

Revert these changes.

@chalmerlowe
Copy link
Collaborator

chalmerlowe commented Oct 5, 2023

Thanks for providing this PR. I am grateful that you are looking to assist.

For the record: work is being conducted in the following PR to address this issue.
I submitted some ideas back to the original author of 1656 to help them advance their code and provide tests. I am gonna finalize that effort. Will close this PR.

#1656

@chalmerlowe chalmerlowe closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tswast tswast tswast requested changes

@jainsahab jainsahab Awaiting requested review from jainsahab jainsahab was automatically assigned from googleapis/api-bigquery

Assignees

No one assigned

Labels

api: bigquery Issues related to the googleapis/python-bigquery API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DevStephanie @chalmerlowe @tswast