feat(config): honor COGNITE_DISABLE_PYPI_VERSION_CHECK for PyPI version warning#2531
feat(config): honor COGNITE_DISABLE_PYPI_VERSION_CHECK for PyPI version warning#2531andersfylling wants to merge 1 commit intomasterfrom
Conversation
…on warning Read the env var when building ClientConfig so deployments can disable the newer-version check without importing global_config first. Truthy values match common conventions (1, true, yes, on). Update the UserWarning text to document the env-based escape hatch. Made-with: Cursor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the SDK's configuration flexibility by allowing users to disable the PyPI version check warning through an environment variable, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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
The pull request introduces the capability to disable the PyPI version check using an environment variable, COGNITE_DISABLE_PYPI_VERSION_CHECK, in addition to the existing global configuration. This involves adding new helper functions, updating configuration logic, and enhancing the warning message. The review identifies that the new helper functions, _env_truthy and _pypi_version_check_disabled, lack docstrings, and _env_truthy uses a magic value that should be refactored into an UPPER_SNAKE_CASE constant, both of which violate the repository's style guide.
| def _env_truthy(name: str) -> bool: | ||
| val = os.environ.get(name) | ||
| if val is None: | ||
| return False | ||
| return val.strip().lower() in {"1", "true", "yes", "on"} |
There was a problem hiding this comment.
This function is missing a docstring, which is required by the repository style guide (lines 94, 117). Additionally, the set of truthy strings is a magic value and should be defined as a constant with an UPPER_SNAKE_CASE name, as per the style guide (line 87).
Since a module-level constant cannot be added within the scope of this change, defining it inside the function is an acceptable alternative.
def _env_truthy(name: str) -> bool:
"""Check if an environment variable is set to a truthy value.
Truthy values are "1", "true", "yes", "on" (case-insensitive).
Args:
name (str): The name of the environment variable.
Returns:
bool: True if the environment variable has a truthy value, False otherwise.
"""
TRUTHY_STRINGS = frozenset({"1", "true", "yes", "on"})
val = os.environ.get(name)
if val is None:
return False
return val.strip().lower() in TRUTHY_STRINGS| def _pypi_version_check_disabled() -> bool: | ||
| return global_config.disable_pypi_version_check or _env_truthy("COGNITE_DISABLE_PYPI_VERSION_CHECK") |
There was a problem hiding this comment.
This function is missing a docstring, which is required by the repository style guide (lines 94, 117).
def _pypi_version_check_disabled() -> bool:
"""Check if the PyPI version check is disabled.
The check is disabled if either the global config flag is set or the
corresponding environment variable is set to a truthy value.
Returns:
bool: True if the version check is disabled, False otherwise.
"""
return global_config.disable_pypi_version_check or _env_truthy("COGNITE_DISABLE_PYPI_VERSION_CHECK")References
- Functions require concise docstrings in google-style format, including
ArgsandReturnssections for clarity. (link)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2531 +/- ##
==========================================
+ Coverage 91.35% 91.40% +0.05%
==========================================
Files 192 192
Lines 26218 26226 +8
==========================================
+ Hits 23951 23972 +21
+ Misses 2267 2254 -13
🚀 New features to boost your workflow:
|
|
To simplify, we could consider just the presence of the env.var. to be thruthy here imo. I also would like the opinion of @erlendvollset, as I remember he removed all these |
2 participants
Summary
Honor
COGNITE_DISABLE_PYPI_VERSION_CHECKwhen instantiatingClientConfig, so deployments can disable the newer-versionUserWarningwithout importingglobal_configfirst.Truthy values:
1,true,yes,on(case-insensitive).Motivation
Downstream projects (e.g. unstructured-search) already set this env var; the SDK previously ignored it and only respected
global_config.disable_pypi_version_check.Changes
config.py:_env_truthy/_pypi_version_check_disabled_version_checker.py: mention env var in warning texttest_config.py: unit tests for env + global precedenceMade with Cursor