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.

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

feat: option to use bq connection without check #460

Merged
merged 10 commits into from
Mar 21, 2024
Merged

Conversation

shobsi
Copy link
Contributor

@shobsi shobsi commented Mar 18, 2024

BQ connection is currently used in BQ remote function and BQML Vertex AI integrations. It has a dependency on resource manager API, which is still going through the process of AXT compliance. We should provide a path in which BigFrames can work without resource manager interaction for the following two reasons:

  1. AXT compliance: By having such path user can invoke a fully AXT compliant GCP interaction
  2. This lowers the IAM bar for the user to use bigframes remote function and llm (which currently require getIamPolcy and setIamPolcy permissions) by managing the BQ connection outside of bigframes.

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes internal issue 330224555 🦕

@shobsi shobsi requested review from a team as code owners March 18, 2024 22:16
@shobsi shobsi requested a review from ashleyxuu March 18, 2024 22:16
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Mar 18, 2024
@shobsi shobsi requested review from GarrettWu and tswast and removed request for ashleyxuu March 18, 2024 22:17
bigframes/ml/llm.py Outdated Show resolved Hide resolved
bigframes/_config/bigquery_options.py Outdated Show resolved Hide resolved
self._bq_connection_id,
"run.invoker",
)
if self._bq_connection_manager:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think of creating a dummy ConnectionManager class, and when skip_bq_connection_check is True, use the dummy implementation instead of a real one. Then the same logic can apply to all the places, and the user (of the connection_manager) doesn't need to think about the implementations.

Well just a suggestion, up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um, gave it a thought, feels weird to implement a class with a method create_bq_connection which doesn't create anything. I'd prefer to not do that. Let me know if you have strong opinion about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we'd rename the create_bq_connection to sth like prepare_bq_connection.

No need to put too much thoughts on it. Just a suggestion.

@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 Mar 20, 2024
@shobsi shobsi merged commit 0b3f8e5 into main Mar 21, 2024
15 checks passed
@shobsi shobsi deleted the shobs-rf-opaq-conn branch March 21, 2024 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants