fix: REST fallback must not re-create the BigQuery Storage client - #19
Merged
Merged
Conversation
_read_df catches a Storage Read API failure and retries with a bare query_job.to_dataframe(). That argument defaults to create_bqstorage_client=True, so whenever google-cloud-bigquery-storage is importable the fallback silently builds its own Storage client — re-attempting the exact thing it is falling back from and re-raising the identical error. This was latent while the package was absent: the import failed first, so the retry had nothing to auto-create and REST genuinely worked. Installing the package in #18 exposed it, and every Phase D died with 403 ... does not have 'bigquery.readsessions.create' permission logged immediately after "using REST download". Verified against the live identity: to_dataframe() fails, to_dataframe(create_bqstorage_client=False) succeeds. Note this is a per-identity IAM question as well as a packaging one. The pipeline's ADC identity is loqman@measurementlab.net, which lacks roles/bigquery.readSessionUser on mlab-collaboration. Until that is granted the Arrow path is unavailable and this degrades to REST — which works, but is slower and much heavier: a 13.7M-row download OOM-killed the worker at 22.1 GB inside a 28 GB cgroup and failed the 2025-08 backfill chunk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first version sequenced job.to_dataframe side effects assuming BigQueryReadClient() would construct successfully. That holds on a machine with application-default credentials but not in CI, where construction raises first and the Arrow call never happens — so the side_effect list lined up differently and three tests failed. They passed locally for the wrong reason. Drive both branches explicitly with a fake google.cloud.bigquery_storage module whose client constructor can be made to raise, covering the two real-world cases (missing ADC, and the 403 when the identity lacks bigquery.readsessions.create). Verified passing both with ambient ADC and with credentials stripped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #18, which exposed a latent bug in the tomography download fallback.
The bug
_read_df(correlation_tomography.py) catches a Storage Read API failure and retries:to_dataframe()defaults tocreate_bqstorage_client=True, so whenevergoogle-cloud-bigquery-storageis importable the fallback builds its own Storage client — re-attempting the very thing it is falling back from, and re-raising the identical error.Latent while the package was absent (the import failed first, so the retry had nothing to auto-create). #18 installed the package and every Phase D died with
403 ... 'bigquery.readsessions.create', logged immediately after "using REST download".Verified against the live identity
to_dataframe()PermissionDenied: 403 ... readsessions.createto_dataframe(create_bqstorage_client=False)This is also an IAM gap
The pipeline's ADC identity is
loqman@measurementlab.net, which lacksroles/bigquery.readSessionUseronmlab-collaboration. Until that is granted the Arrow path is unavailable and this degrades to REST — which works, but is slower and much heavier: a 13.7M-row download OOM-killed the worker at 22.1 GB inside a 28 GB cgroup, failing the 2025-08 backfill chunk. Granting the role would both fix that OOM and cut ~20 min off every nightly Phase D.Testing
4 new tests in
test_read_df_fallback.py, including a regression test that fails against the old code (the fallback's kwargs were empty). Full suite59 passed, 2 skippedon 3.14; ruff check and format clean.🤖 Generated with Claude Code