test: verify read_gbq_colab label preservation in anywidget display mode - #17887
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the job configuration label creation to prioritize the 'read_gbq_colab' API method as the primary 'bigframes-api' label, and adds a unit test to verify this behavior in anywidget mode. The reviewer recommends converting the 'api_methods' parameter to a local list before mutating it to prevent unintended side effects on the caller and avoid potential runtime errors if a non-list iterable is passed.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
The failed importer test does not related to the change in current branch. |
| (i for i, m in enumerate(api_methods) if "read_gbq_colab" in m), None | ||
| ) | ||
| if colab_idx is not None: | ||
| job_configs_labels["bigframes-api"] = api_methods.pop(colab_idx) |
There was a problem hiding this comment.
It's not immediately clear to me why we need to pop at the colab_idx instead of at head. Could you add some explanatory comments here?
There was a problem hiding this comment.
We have reverted the method-specific popping logic. We used to want to override the label precedence in Python due to the tracking mechanism of dashboard. However, we toss this approach for now, and I will update the change in dashboard instead.
| colab_idx = next( | ||
| (i for i, m in enumerate(api_methods) if "read_gbq_colab" in m), None | ||
| ) | ||
| if colab_idx is not None: | ||
| job_configs_labels["bigframes-api"] = api_methods.pop(colab_idx) | ||
| else: | ||
| job_configs_labels["bigframes-api"] = api_methods[0] | ||
| del api_methods[0] |
There was a problem hiding this comment.
I don't understand why we need a special case for read_gbq_colab. It should be the most recent API call before we execute a requests, so it should always be preserved. Could you double-check the tests to make sure we drop only the oldest labels when we have too many?
There was a problem hiding this comment.
Agreed. We have removed this special case. In this PR, we just make sure session-read_gbq_colab is anyiwdget mode is preserved. I will make the changes from dashboard to loop through all labels. This approach would avoid hardcoded specials cases.
shuoweil
changed the title
faf9492 to
9185cdd
Compare
| job_configs_labels = dict(job_configs_labels) | ||
|
|
||
| if api_methods and "bigframes-api" not in job_configs_labels: | ||
| api_methods = list(api_methods) |
There was a problem hiding this comment.
Non-rhetorical question: do we need this line of change? It alters the behavior of the original code, which alters the original list that was passed in at line 73.
There was a problem hiding this comment.
Yes, this shallow copy is intentional as a safety guard. There are two reasons. (1) at line 73, del api_methods[0]
removes the primary API label. Creating a copy ensures this deletion stys local to the helper function and does not mutate the caller's original list if reused or logs elsewhere.
(2) api_methods may sometime pass in tuple. We have a unit test reate_job_configs_labels(labels, ("read_gbq_colab", "head")) will pass tuples to label. In this case, tuple is immutable. This copy is a guard to avoid errors for del api_methods[0].
3 participants
This PR ensures that
read_gbq_colabtelemetry labels are reliably preserved across BigQuery job configurations when evaluating DataFrames inanywidgetdisplay mode.Fixes #<538249690> 🦕