Skip to content
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

fix: add detailed message in job error #1762

Merged
merged 9 commits into from Jan 10, 2024
Merged

Conversation

Linchin
Copy link
Contributor

@Linchin Linchin commented Jan 8, 2024

Temp fix of internal issue b/310544564.

Starting Oct 2023, customers have found that the error message with jobs have lacked exact cause info. This seems to have been caused by detailed errors being moved to a different part of the job message. This PR makes a temp fix by combining both error sources into the exception we raise.

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)

@Linchin Linchin requested review from a team as code owners January 8, 2024 21:22
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Jan 8, 2024
@Linchin Linchin requested review from chalmerlowe and parthea and removed request for PhongChuong January 8, 2024 21:30
@Linchin Linchin requested a review from a team as a code owner January 8, 2024 23:25
@@ -24,7 +24,7 @@ def main() -> "bigquery.Client":

# [START bigquery_client_json_credentials]
from google.cloud import bigquery
from google.oauth2 import service_account
from google.oauth2 import service_account # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

@Linchin

Thanks for trying to enable the work to continue moving forward.
I saw an error in the mypy-samples nox session in my PR last night with this line of code.

I worry about this as a "fix" for the problem.
That line of code used to work and doesn't now... not only in this repo, but others so I am not comfortable with just ignoring that line without understanding more about the root cause.

I spoke with Anthonios last night. It appears that there is an issue with the types-protobuf library that causes the mypy-samples error. A change was recently made in that library which broke some things downstream.

I am gonna open a separate PR to set limitations for the types-protobuf module to avoid the problematic version of types-protobuf and provide a comment as to why that version is being avoided.

I am hoping that will fix the issue in a slightly more robust way than simply ignoring the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR 1764 was approved. I merged that code into main and into this branch. Hopefully that should clear up the mypy-samples error and we can delete the type: ignore comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for explaining the issue in detail! It was indeed strange, because there wasn't much change in the imported module google.oauth2. I'll remove the ignore here.

Copy link
Contributor

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

See some comments in the code and a thought:

I recommend that we add a test to ensure that this code works as desired AND to help ensure that in the future if we modify this code, that we don't break anything.

google/cloud/bigquery/job/base.py Outdated Show resolved Hide resolved
google/cloud/bigquery/job/base.py Outdated Show resolved Hide resolved
google/cloud/bigquery/job/base.py Outdated Show resolved Hide resolved
Linchin and others added 2 commits January 9, 2024 12:56
Co-authored-by: Chalmer Lowe <chalmer.lowe@gmail.com>
Co-authored-by: Chalmer Lowe <chalmer.lowe@gmail.com>
@chalmerlowe
Copy link
Contributor

I made a few changes/suggestions via the web interface to Github and my personal email address got associated with them so that is why the CLA error is showing up.
Sigh.

@Linchin
Copy link
Contributor Author

Linchin commented Jan 9, 2024

It happened to me last time too! I just overrode it :)

Copy link
Contributor

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM

@Linchin Linchin merged commit 08483fb into googleapis:main Jan 10, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants