feat: add pandas-gbq capability helper - #17957
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 introduces the PandasGBQVersions helper class to manage versioning and delegation API support for the pandas-gbq package, along with comprehensive unit tests. However, the properties installed_version and delegation_api_version check for cached values but fail to actually assign and store the parsed values into their respective cache variables (self._installed_version and self._delegation_api_version), defeating the caching mechanism.
|
@shuoweil , Please see #17958 (comment) for the |
Thanks, Anthonios. |
|
I need to merge #17964 first. It contains testcase fixed. |
…_helpers.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…_helpers.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1431979 to
8d6425a
Compare
| return self._installed_version | ||
|
|
||
| @property | ||
| def delegation_api_version(self) -> int: |
There was a problem hiding this comment.
Maybe we should use packaing.version.Version instead of int for the return type? Will the delegation api version follow the version format of major.minor.patch ?
| with mock.patch.dict(sys.modules, {"google.cloud.bigquery_storage": None}): | ||
| with mock.patch.dict(cloud.__dict__): |
There was a problem hiding this comment.
nit: shall we chain the context managers with a single with? Same suggestion applies to other places.
| def test_pandas_gbq_is_delegation_supported_true(): | ||
| versions = _versions_helpers.PandasGBQVersions() | ||
| versions._delegation_api_version = 1 | ||
| assert versions.is_delegation_supported is True |
There was a problem hiding this comment.
super nit: let's place an empty line above the assertion.
| def test_pandas_gbq_is_delegation_supported_false(): | ||
| versions = _versions_helpers.PandasGBQVersions() | ||
| versions._delegation_api_version = 0 | ||
| assert versions.is_delegation_supported is False |
There was a problem hiding this comment.
super nit: let's add an empty line above the assertion.
| def __init__(self): | ||
| self._installed_version = None | ||
| self._delegation_api_version = None |
There was a problem hiding this comment.
[No action need in this PR] It seems we are following the existing pattern in the codebase, but it is not very "Pythonic" as it uses classes excessively.
I think ideally we should replace all these classes and lazy evaluations with top-level functions and eager evaluations. We should also return None in case the package is not import-able.
We may consider creating a bug for a codebase cleanup, and loop in @tswast for his thoughts on this.
| def test_installed_pandas_gbq_version_returns_cached(): | ||
| versions = _versions_helpers.PandasGBQVersions() | ||
| versions._installed_version = object() | ||
| assert versions.installed_version is versions._installed_version |
There was a problem hiding this comment.
super nit: let's place an empty line above to demarcate "action" and "assert" blocks: go/unit-testing-practices?polyglot=java#structure
4 participants
Introduces
PandasGBQVersionsin_versions_helpers.pyto support backend delegation and telemetry:PandasGBQVersionswithinstalled_version,delegation_api_version, andis_delegation_supportedproperties.pandas-gbqinstallations gracefully by defaulting to version0.0.0/ delegation version0.test__versions_helpers.py.Fixes #<540939659> 🦕