-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: support automatic per-cell execution history filtering and isolated callbacks #17144
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
Changes from 21 commits
d3bf48c
4258639
7574f22
eccd236
d2eea39
21d5dd8
97c7260
f080dda
4bd2735
9dd03ed
dcae615
eb548b6
7db1330
da6682c
0ef9216
66fe150
b9c9336
5f9d824
d9a5593
0c8f8c7
81bf807
409a2fb
f158662
eb3934b
ee477b8
0883502
b04eb14
430de34
a82efbe
d6b1bc1
c2bfb9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| import threading | ||
| import traceback | ||
| import warnings | ||
| from typing import TYPE_CHECKING, Callable, Optional, TypeVar | ||
| from typing import TYPE_CHECKING, Callable, Iterable, Optional, TypeVar | ||
|
|
||
| import google.auth.exceptions | ||
|
|
||
|
|
@@ -124,12 +124,22 @@ def with_default_session(func_: Callable[..., _T], *args, **kwargs) -> _T: | |
| return func_(get_global_session(), *args, **kwargs) | ||
|
|
||
|
|
||
| def execution_history() -> "bigframes.session._ExecutionHistory": | ||
| def execution_history( | ||
| *, | ||
| events: Optional[Iterable[bigframes.core.events.Event]] = None, | ||
| job_ids: Optional[Iterable[str]] = None, | ||
| all_cells: bool = True, | ||
| ) -> "bigframes.session._ExecutionHistory": | ||
| import pandas # noqa: F401 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this is needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the unused and redundant import pandas # noqa: F401 statement in packages/bigframes/bigframes/core/global_session.py under the execution_history function. |
||
|
|
||
| import bigframes.session | ||
|
|
||
| return with_default_session(bigframes.session.Session.execution_history) | ||
| return with_default_session( | ||
| bigframes.session.Session.execution_history, | ||
| events=events, | ||
| job_ids=job_ids, | ||
| all_cells=all_cells, | ||
| ) | ||
|
|
||
|
|
||
| class _GlobalSessionContext: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,3 +249,16 @@ def timedelta_to_micros( | |
| ) * 1_000_000 + timedelta.microseconds | ||
|
|
||
| raise TypeError(f"Unrecognized input type: {type(timedelta)}") | ||
|
|
||
|
|
||
| def get_ipython_execution_count() -> typing.Optional[int]: | ||
| """Returns the current IPython cell execution count if running in a notebook, else None.""" | ||
| try: | ||
| import IPython | ||
|
|
||
| ipy = IPython.get_ipython() | ||
| if ipy is not None and hasattr(ipy, "execution_count"): | ||
| return ipy.execution_count | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't rely on monkey patching, may be consider some existing settings or properties.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! I refactored this to use the standard InteractiveShell.initialized() and InteractiveShell.instance() properties instead of IPython.get_ipython(). This also let me clean up the unit tests to avoid mocking sys.modules. |
||
| except (ImportError, NameError): | ||
| pass | ||
| return None | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,6 +300,7 @@ def _try_read_gbq_colab_sessionless_dry_run( | |
| def _read_gbq_colab( # type: ignore[overload-overlap] | ||
| query_or_table: str, | ||
| *, | ||
| callback: Optional[Callable[[bigframes.core.events.EventEnvelope], None]] = ..., | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why default is ... ? Doesn't comply with type hint.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Standardized the overloads of _read_gbq_colab in packages/bigframes/bigframes/pandas/io/api.py. The default values in the overloads are now None and False to explicitly match their type annotations (Optional[...] and Literal[False]) and prevent any potential type checker warnings. |
||
| pyformat_args: Optional[Dict[str, Any]] = ..., | ||
| dry_run: Literal[False] = ..., | ||
| ) -> bigframes.dataframe.DataFrame: ... | ||
|
|
@@ -309,6 +310,7 @@ def _read_gbq_colab( # type: ignore[overload-overlap] | |
| def _read_gbq_colab( | ||
| query_or_table: str, | ||
| *, | ||
| callback: Optional[Callable[[bigframes.core.events.EventEnvelope], None]] = ..., | ||
| pyformat_args: Optional[Dict[str, Any]] = ..., | ||
| dry_run: Literal[True] = ..., | ||
| ) -> pandas.Series: ... | ||
|
|
@@ -317,6 +319,7 @@ def _read_gbq_colab( | |
| def _read_gbq_colab( | ||
| query_or_table: str, | ||
| *, | ||
| callback: Optional[Callable[[bigframes.core.events.EventEnvelope], None]] = None, | ||
| pyformat_args: Optional[Dict[str, Any]] = None, | ||
| dry_run: bool = False, | ||
| ) -> bigframes.dataframe.DataFrame | pandas.Series: | ||
|
|
@@ -328,6 +331,8 @@ def _read_gbq_colab( | |
| Args: | ||
| query_or_table (str): | ||
| SQL query or table ID (table ID not yet supported). | ||
| callback (Optional[Callable[[bigframes.core.events.EventEnvelope], None]]): | ||
| Callback to receive query execution events. | ||
| pyformat_args (Optional[Dict[str, Any]]): | ||
| Parameters to format into the query string. | ||
| dry_run (bool): | ||
|
|
@@ -379,6 +384,7 @@ def _read_gbq_colab( | |
| return global_session.with_default_session( | ||
| bigframes.session.Session._read_gbq_colab, | ||
| query_or_table, | ||
| callback=callback, | ||
| pyformat_args=pyformat_args, | ||
| dry_run=dry_run, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't figure out what does it mean by just reading the params. Add doc strings to explain this and other parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a clear, comprehensive docstring to the EventEnvelope class in packages/bigframes/bigframes/core/events.py. It now explicitly documents what the wrapper does and describes each parameter, including how cell_execution_count is captured and used to filter and scope execution history on a per-cell basis.