Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add support for BigQuery DataFrames. Set context.engine to 'bigframes' to support query results larger than 10 GB #58

Merged
merged 18 commits into from
Sep 20, 2024
Merged

feat: add support for BigQuery DataFrames. Set context.engine to 'bigframes' to support query results larger than 10 GB #58

merged 18 commits into from
Sep 20, 2024

Conversation

sycai
Copy link
Contributor

@sycai sycai commented Sep 19, 2024

No description provided.

@sycai sycai requested review from a team as code owners September 19, 2024 00:13
@sycai sycai requested a review from tswast September 19, 2024 00:13
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 19, 2024
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-magics API. label Sep 19, 2024
@sycai sycai changed the title feat: support BigFrames API by setting context.engine to 'bigframes' feat: support BigFrames queries by setting context.engine to 'bigframes' Sep 19, 2024
tswast
tswast reviewed Sep 19, 2024
View reviewed changes
setup.py Outdated
@@ -28,6 +28,7 @@
# 'Development Status :: 5 - Production/Stable'``
release_status = "Development Status :: 4 - Beta"
dependencies = [
"bigframes >= 1.17.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't.

In the design I shared that this should be an optional dependency. That means it should be an "extra".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Moved it to extra.

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 19, 2024
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 19, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 19, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 19, 2024
@sycai sycai requested a review from tswast September 19, 2024 19:58
tswast
tswast reviewed Sep 19, 2024
View reviewed changes
owlbot.py Outdated
@@ -56,6 +56,7 @@
# Multi-processing note isn't relevant, as bigquery-magics is responsible for
# creating clients, not the end user.
"docs/multiprocessing.rst",
"noxfile.py",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't modify the noxfile in this way. There is a feature in the template that does what we want.

https://github.com/googleapis/synthtool/blob/master/synthtool/gcp/templates/python_library/noxfile.py.j2#L75C1-L75C27

Use unit_test_extras_by_python to make sure we have some Python versions where we test without extras and some where we test with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change reverted

tswast
tswast reviewed Sep 19, 2024
View reviewed changes
noxfile.py Outdated
@@ -217,6 +224,9 @@ def unit(session, protobuf_implementation):
if protobuf_implementation == "cpp":
session.install("protobuf<4")

if install_bigframes:
session.install("bigframes >= 1.17.0")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the "extras". Don't install this manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Updated owlbot.py instead

tswast
tswast reviewed Sep 19, 2024
View reviewed changes
setup.py
@@ -54,6 +54,7 @@
"grpcio >= 1.47.0, < 2.0dev",
"grpcio >= 1.49.1, < 2.0dev; python_version>='3.11'",
],
"bigframes": ["bigframes >= 1.17.0"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pip will always install the latest version of bigframes. Let's make sure we test our compatibility with the minimum supported bigframes by adding bigframes==1.17.0 to https://github.com/googleapis/python-bigquery-magics/blob/main/testing/constraints-3.9.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@sycai sycai requested a review from tswast September 19, 2024 22:17
tswast
tswast approved these changes Sep 20, 2024
View reviewed changes
Copy link
Collaborator

@tswast tswast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

bigquery_magics/bigquery.py
Comment on lines +454 to +455
bpd.options.bigquery.project = context.project
bpd.options.bigquery.credentials = context.credentials
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried these might fail if the bigframes session has already started. Maybe that's what we want to happen?

Alternatively, we could explicitly create a bigframes Session object, but then whatever DataFrame we produce won't be compatible with other DataFrames in the notebook.

@tswast tswast changed the title feat: support BigFrames queries by setting context.engine to 'bigframes' feat: add support for BigQuery DataFrames. Set context.engine to 'bigframes' to support query results larger than 10 GB Sep 20, 2024
@tswast tswast merged commit 90ba05f into main Sep 20, 2024
24 checks passed
@tswast tswast deleted the issue16-context branch September 20, 2024 21:36
@release-please release-please bot mentioned this pull request Sep 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tswast tswast tswast approved these changes

Assignees

@TrevorBergeron TrevorBergeron

Labels
api: bigquery Issues related to the googleapis/python-bigquery-magics API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sycai @tswast @TrevorBergeron