fix: Respect display.progress_bar=None in background threads#16715
fix: Respect display.progress_bar=None in background threads#16715shuoweil wants to merge 28 commits into
Conversation
shuoweil
requested review from
GarrettWu and
sycai
and removed request for
a team and
sycai
shuoweil
force-pushed
the
shuowei-anywidget-extraneous-output
branch
from
de201ee to
d887714
Compare
![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 introduces a mechanism to pass the progress_bar configuration through BigQuery events, allowing for more granular control over progress bar display. It updates several event classes to include a progress_bar field and modifies the callback logic to prioritize this field over global settings. Feedback suggests defining the 'fallback_to_global' sentinel as a constant to improve maintainability and refactoring the event mapping logic in create_bq_event_callback to reduce code duplication.
shuoweil
force-pushed
the
shuowei-anywidget-extraneous-output
branch
from
dce2e04 to
42e4a50
Compare
shuoweil
requested review from
TrevorBergeron,
chelsea-lin and
sycai
and removed request for
sycai
|
This PR doesn't touch the thread localility of the configuration object. Please continue with this fix. |
|
|
||
| import bigframes.session.executor | ||
|
|
||
| _FALLBACK_TO_GLOBAL = "fallback_to_global" |
There was a problem hiding this comment.
Generally "default" would communicate this more succinctly.
There was a problem hiding this comment.
Done. Replaced _FALLBACK_TO_GLOBAL with _DEFAULT = "default".
shuoweil
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
tswast
left a comment
tswast
left a comment
There was a problem hiding this comment.
Please sync with main so that the tests can re-run.
| import google.cloud.bigquery._job_helpers | ||
| import google.cloud.bigquery.job.query | ||
| import google.cloud.bigquery.table | ||
| from google.cloud.bigquery.job.query import QueryPlanEntry |
There was a problem hiding this comment.
Use the fully qualified name below. According to Google style, import modules not classes or functions.
| location: Optional[str] = None | ||
| job_id: Optional[str] = None | ||
| request_id: Optional[str] = None | ||
| progress_bar: Optional[str] = _DEFAULT |
There was a problem hiding this comment.
Since we are 3.10+ now, we can use the recommended | None instead of Optional for clarity.
| progress_bar: Optional[str] = _DEFAULT | |
| progress_bar: str| None = _DEFAULT |
Also, instead of general str, consider using Literal with the allowed values.
| def from_bqclient( | ||
| cls, | ||
| event: google.cloud.bigquery._job_helpers.QuerySentEvent, | ||
| progress_bar: Optional[str] = _DEFAULT, |
There was a problem hiding this comment.
Same here, | None and potentially Literal as well.
| location: str | None = None | ||
| job_id: str | None = None | ||
| request_id: str | None = None | ||
| progress_bar: Literal["default", "auto", "notebook", "terminal"] | None = _DEFAULT |
There was a problem hiding this comment.
"progress_bar" isn't really state about the event, is it? Could you describe to me again what this is trying to solve?
Consider an alternative where the progress bar type is attached to the subscriber, instead, as that seems to be more appropriate. Or another alternative in which we wrap the events with some sort of "CellState" that gives an indicator of the current execution context, of which the progress_bar could be a property of that.
There was a problem hiding this comment.
Hi Tim, thanks for the suggestions.
I looked into the first alternative (attaching the progress bar type to the subscriber), but it doesn't quite work for what we are trying to solve. The progress_bar option is thread-local and needs to be captured from the initiating thread at the moment the query starts. Since subscribers are typically long-lived (session-scoped), they cannot easily capture this per-query context. Furthermore, the callbacks from the BigQuery client library run in a background thread where the initiating thread's local storage is not accessible.
Therefore, I took your second alternative: I removed progress_bar from the event classes and instead wrapped the events in an EventEnvelope (acting as the execution context/'CellState' you suggested). This envelope is created in the callback (which captures the initiating thread's option via a closure) and carries the progress_bar setting along with the event to the subscribers.
This keeps the events clean of display preferences while solving the cross-thread propagation issue. Thanks a lot.
3 participants
Captures the progress bar option from the initiating thread and propagates it via events to background callbacks, ensuring options like None are respected.
Fixes #<461829560> 🦕