- 
                Notifications
    You must be signed in to change notification settings 
- Fork 64
feat: use default session connection #87
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 1 commit
4d17229
              b0e9215
              dd05552
              6739428
              4ee895e
              dcf93fa
              c79c86e
              62ac080
              cd6c86a
              71ec5b0
              aa42ff7
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -29,6 +29,8 @@ | |
| ) | ||
| logger = logging.getLogger(__name__) | ||
|  | ||
| _BIGFRAMES_DEFAULT_CONNECTION_ID = "bigframes-default-connection" | ||
|  | ||
|  | ||
| class BqConnectionManager: | ||
| """Manager to handle operations with BQ connections.""" | ||
|  | @@ -162,3 +164,25 @@ def _get_service_account_if_connection_exists( | |
| pass | ||
|  | ||
| return service_account | ||
|  | ||
|  | ||
| def get_connection_name_full( | ||
| 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. [nit] since it takes a connection and returns a connection after resolving the format, would prefer naming it  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. Will address in another PR. | ||
| connection_name: Optional[str], default_project: str, default_location: str | ||
| 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. awkward to have optional argument followed by mandatory arguments, would be nice to have it in the end 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. Optional only means it can be value None. It is OK to put it at the front, as it is the most important param of the function. Not really "optional", which means it has a default value. Then it has to be at the end. | ||
| ) -> str: | ||
| """Retrieve the full connection name of the form <PROJECT_NUMBER/PROJECT_ID>.<LOCATION>.<CONNECTION_ID>. | ||
| Use default project, location or connection_id when any of them are missing.""" | ||
| if connection_name is None: | ||
| return ( | ||
| f"{default_project}.{default_location}.{_BIGFRAMES_DEFAULT_CONNECTION_ID}" | ||
| ) | ||
|  | ||
| if connection_name.count(".") == 2: | ||
| return connection_name | ||
|  | ||
| if connection_name.count(".") == 1: | ||
| return f"{default_project}.{connection_name}" | ||
|  | ||
| if connection_name.count(".") == 0: | ||
| return f"{default_project}.{default_location}.{connection_name}" | ||
|  | ||
| raise ValueError(f"Invalid connection name format: {connection_name}.") | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -49,7 +49,14 @@ def __init__( | |
| connection_name: Optional[str] = None, | ||
| ): | ||
| self.session = session or bpd.get_global_session() | ||
| self.connection_name = connection_name or self.session._bq_connection | ||
|  | ||
| connection_name = connection_name or self.session._bq_connection | ||
| 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. I thought about this a bit, we are resolving between 3 potential sources of connection: 
 How about we set the default in   self._bq_connection = context.bq_connection or "bigframes-default-connection"We are assuming other session defaults there, such as  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. Oh yea, I did this way at first place, but then I found remote_function doesn't always have a session object. So I had to put the default away from session. Then we added default session... We can move it to session. | ||
| self.connection_name = clients.get_connection_name_full( | ||
| connection_name, | ||
| default_project=self.session._project, | ||
| default_location=self.session._location, | ||
| ) | ||
|  | ||
| self._bq_connection_manager = clients.BqConnectionManager( | ||
| self.session.bqconnectionclient, self.session.resourcemanagerclient | ||
| ) | ||
|  | @@ -181,7 +188,14 @@ def __init__( | |
| connection_name: Optional[str] = None, | ||
| ): | ||
| self.session = session or bpd.get_global_session() | ||
| self.connection_name = connection_name or self.session._bq_connection | ||
|  | ||
| connection_name = connection_name or self.session._bq_connection | ||
| self.connection_name = clients.get_connection_name_full( | ||
| connection_name, | ||
| default_project=self.session._project, | ||
| default_location=self.session._location, | ||
| ) | ||
|  | ||
| self._bq_connection_manager = clients.BqConnectionManager( | ||
| self.session.bqconnectionclient, self.session.resourcemanagerclient | ||
| ) | ||
|  | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|  | @@ -465,6 +465,36 @@ def square(x): | |||
| assert_pandas_df_equal_ignore_ordering(bf_result, pd_result) | ||||
|  | ||||
|  | ||||
| @pytest.mark.flaky(retries=2, delay=120) | ||||
| def test_remote_function_default_connection(scalars_dfs, dataset_id): | ||||
| @rf.remote_function([int], int, dataset=dataset_id) | ||||
|          | ||||
| This is an internal method. Please use :func:`bigframes.pandas.remote_function` instead. | 
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.
Changed to bigframes.pandas.remote_function.
But in that case, if all the use cases are through session.remote_function or bigframes.pandas.remote_function, then it will be always the case that rf.remote_function's input session is not None. And we won't need to resolve to bpd.get_global_session() in rf.remote_function.
I'll leave it for you to resolve.
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.
I think it's okay, the goal of this test is to assert that default connection takes effect irrespective of the session.
We can add a separate large test test_remote_function_direct_defaults_to_global_session specifically for that change.
|         
                  shobsi marked this conversation as resolved.
              Show resolved
            Hide resolved | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # Copyright 2023 Google LLC | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|  | ||
| from bigframes import clients | ||
|  | ||
|  | ||
| def test_get_connection_name_full_none(): | ||
| connection_name = clients.get_connection_name_full( | ||
| None, default_project="default-project", default_location="us" | ||
| ) | ||
| assert connection_name == "default-project.us.bigframes-default-connection" | ||
|  | ||
|  | ||
| def test_get_connection_name_full_connection_id(): | ||
| connection_name = clients.get_connection_name_full( | ||
| "connection-id", default_project="default-project", default_location="us" | ||
| ) | ||
| assert connection_name == "default-project.us.connection-id" | ||
|  | ||
|  | ||
| def test_get_connection_name_full_location_connection_id(): | ||
| connection_name = clients.get_connection_name_full( | ||
| "eu.connection-id", default_project="default-project", default_location="us" | ||
| ) | ||
| assert connection_name == "default-project.eu.connection-id" | ||
|  | ||
|  | ||
| def test_get_connection_name_full_all(): | ||
| connection_name = clients.get_connection_name_full( | ||
| "my-project.eu.connection-id", | ||
| default_project="default-project", | ||
| default_location="us", | ||
| ) | ||
| assert connection_name == "my-project.eu.connection-id" | 
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.
Would it be tidier to make it a class method in
BqConnectionManager?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.
I think the function is only a helper that does string manipulation. Instead of the BqConnectionManager which contains the states of the clients. So separate them.
Uh oh!
There was an error while loading. Please reload this page.
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.
IMHO it would still make sense. A
@classmethodwould mean that it does not depend on the instance level state. But conceptually it would fit in nicely - BqConnectionManager sounds like the entity which is supposed to know the low level details of a connection and can provide helper functions about the same.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.
That could work. Will address in another PR.