refactor(bigframes): make executor internals async#17093
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 refactors the execution engine to support asynchronous operations using asyncio. It introduces a background event loop and converts core executor methods to async, while wrapping blocking I/O calls with asyncio.to_thread. Review feedback identifies a missing threading import, a regression where the removed prepare_plan method is still called in _is_trivially_executable, and suggestions to add docstrings to the newly refactored planning methods.
| subscribers_snapshot = list(self._subscribers) | ||
| loop = asyncio.get_running_loop() | ||
| tasks = [ | ||
| loop.run_in_executor(self._executor, subscriber, event) |
There was a problem hiding this comment.
This may eventually cause trouble. I've experienced having the main thread be the only one that can apply GUI updates in some frameworks, but I suppose we can address that (e.g. by having an option or something to run on the same thread as the original), but should be fine for the current use case of printing to stdout/stderr.
|
|
||
| # If the user has labels they wish to set, make sure we set those first so | ||
| # they are preserved. | ||
| for key, value in bigframes.options.compute.extra_query_labels.items(): |
There was a problem hiding this comment.
Did we move this feature somewhere else?
Edit: Yes. ExecutionSpec now reads this config.
2 participants
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:
Fixes #<issue_number_goes_here> 🦕