refactor(config): typed runtime settings via pydantic-settings - #1098
refactor(config): typed runtime settings via pydantic-settings#1098Aaron ("AJ") Steers (aaronsteers) wants to merge 4 commits into
Conversation
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1785891632-pyairbyte-settings' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1785891632-pyairbyte-settings'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| default=os.getenv("CI", "false"), | ||
| ) | ||
| ) | ||
| AIRBYTE_PRINT_FULL_ERROR_LOGS: bool = _SETTINGS.print_full_error_logs |
There was a problem hiding this comment.
🚫 Not fixing. False positive — AIRBYTE_PRINT_FULL_ERROR_LOGS is imported and used in airbyte/exceptions.py:48,83 (print_full_log: bool = AIRBYTE_PRINT_FULL_ERROR_LOGS). Usage is cross-module, which this file-scoped check doesn't see. The CI-derived default is preserved via default_factory=lambda: _str_to_bool(os.getenv("CI", "false")), since env_prefix can't express an unprefixed fallback var.
| """ | ||
|
|
||
| NO_UV: bool = os.getenv("AIRBYTE_NO_UV", "").lower() not in {"1", "true", "yes"} | ||
| NO_UV: bool = _SETTINGS.no_uv |
There was a problem hiding this comment.
🚫 Not fixing. False positive — NO_UV is imported and used in airbyte/validate.py:133,138 and airbyte/_executors/python.py:131,134,137,160. Cross-module usage again. Worth noting for human reviewers: NO_UV is inverted relative to its name (it is True unless AIRBYTE_NO_UV is 1/true/yes), which contradicts its own docstring. This PR preserves that behavior deliberately and pins it with a characterization test rather than changing it in a refactor.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| default=os.getenv("CI", "false"), | ||
| ) | ||
| ) | ||
| AIRBYTE_PRINT_FULL_ERROR_LOGS: bool = _SETTINGS.print_full_error_logs |
| """ | ||
|
|
||
| NO_UV: bool = os.getenv("AIRBYTE_NO_UV", "").lower() not in {"1", "true", "yes"} | ||
| NO_UV: bool = _SETTINGS.no_uv |
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Pull request overview
Refactors PyAirbyte’s non-secret runtime configuration to use a typed pydantic-settings model, preserving the existing “snapshot at import time” semantics in airbyte.constants while adding optional airbyte.yaml / airbyte.toml config-file support (env > file > defaults) without touching the secrets subsystem.
Changes:
- Added
airbyte/settings.py(BaseSettings) to parse the eight non-secret runtime settings from env + optional YAML/TOML files. - Updated
airbyte/constants.pyto derive constants from a singleSettings()instance created at import time. - Added unit tests and documentation, and introduced
pydantic-settings[yaml]as a dependency.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Locks pydantic-settings (with yaml extra) into the resolved dependency set. |
pyproject.toml |
Adds pydantic-settings[yaml] to project dependencies. |
README.md |
Documents optional airbyte.yaml / airbyte.toml runtime settings and precedence. |
airbyte/settings.py |
Introduces typed, source-ordered runtime settings loading via pydantic-settings. |
airbyte/constants.py |
Replaces os.getenv parsing with a Settings() snapshot at import time. |
tests/unit_tests/test_settings.py |
Adds unit tests for config loading, precedence, and legacy parsing quirks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _parse_no_uv(value: object) -> bool: | ||
| """Preserve the inverted legacy AIRBYTE_NO_UV behavior.""" | ||
| if isinstance(value, bool): | ||
| return value | ||
| return str(value).lower() not in {"1", "true", "yes"} |
There was a problem hiding this comment.
🙋 Human Input Needed: Correct catch, and it exposes a genuine ambiguity I don't want to resolve unilaterally.
The root cause is that NO_UV is inverted relative to its own name and docstring on main: AIRBYTE_NO_UV=true sets NO_UV = False, i.e. the string true means "do use uv". So a boolean in a config file has two defensible meanings:
- Mirror the env var literally (your reading):
no_uv: truebehaves exactly likeAIRBYTE_NO_UV=true→NO_UV == False. Consistent across sources for the same key, but a user readingno_uv: truein their YAML and getting uv enabled will reasonably file a bug. - Honor the field's meaning:
no_uv: true→NO_UV == True, matching whatconstants.NO_UVactually controls, but then the same key means opposite things depending on whether it came from env or file — which is worse.
Neither is right while the underlying inversion stands. My preference is to fix the inversion itself in a follow-up (so AIRBYTE_NO_UV=true means "no uv", as documented) and make the file source a plain mirror — but that is a behavior change for existing users, so it is Aaron ("AJ") Steers (@aaronsteers)'s call. I've asked him; holding this thread until he decides rather than baking in a guess.
| def test_toml_config_file_values_are_loaded( | ||
| tmp_path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| monkeypatch.chdir(tmp_path) | ||
| (tmp_path / "airbyte.toml").write_text( | ||
| 'offline_mode = true\ncache_root = "/from-toml"\n' | ||
| ) | ||
|
|
||
| settings = Settings() | ||
|
|
||
| assert settings.offline_mode is True | ||
| assert settings.cache_root == Path("/from-toml") | ||
|
|
There was a problem hiding this comment.
🙋 Human Input Needed: Deliberately omitted, for the reason in the _parse_no_uv thread — what a file-provided no_uv boolean should mean is undecided, so a characterization test would pin behavior that may be wrong. The env-var string inversion is covered because that path is unambiguously the existing behavior on main. I'll add the file-boolean case as soon as Aaron ("AJ") Steers (@aaronsteers) picks a direction.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPyAirbyte adds typed runtime settings from environment variables, optional ChangesRuntime settings configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Environment
participant ConfigFile as airbyte.yaml/TOML
participant Settings
participant Constants as airbyte.constants
Environment->>Settings: Provide initialization and AIRBYTE_ values
ConfigFile->>Settings: Provide optional file values
Settings->>Settings: Apply precedence and parse typed fields
Settings->>Constants: Supply runtime configuration values
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@airbyte/constants.py`:
- Around line 135-140: Update the TEMP_FILE_CLEANUP docstring to document both
supported configuration sources: the temp_file_cleanup YAML/TOML setting and the
AIRBYTE_TEMP_FILE_CLEANUP environment variable. Describe their precedence
accurately, including the default True behavior when neither source is
configured.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2823200-57f5-40da-9acf-4c67a44edb63
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
README.mdairbyte/constants.pyairbyte/settings.pypyproject.tomltests/unit_tests/test_settings.py
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
airbyte/constants.py:105
- Docstring for DEFAULT_CACHE_ROOT only mentions the AIRBYTE_CACHE_ROOT env var, but the value can now also come from airbyte.yaml/airbyte.toml. Update the docstring so it matches the new settings sources.
)
"""Default cache root is `.cache` in the current working directory.
Values use `AIRBYTE_CACHE_ROOT`, then `cache_root` from `airbyte.yaml` or `airbyte.toml`, then the
airbyte/constants.py:129
- TEMP_DIR_OVERRIDE’s docstring says the value is read from AIRBYTE_TEMP_DIR only, but this PR routes it through Settings which can also load from airbyte.yaml/airbyte.toml. The docstring should mention both sources to avoid confusion.
TEMP_DIR_OVERRIDE: Path | None = _SETTINGS.temp_dir
"""The directory to use for temporary files.
Values use `AIRBYTE_TEMP_DIR`, then `temp_dir` from `airbyte.yaml` or `airbyte.toml`, then the
airbyte/constants.py:140
- TEMP_FILE_CLEANUP’s docstring says the value is read from AIRBYTE_TEMP_FILE_CLEANUP only, but it can now also be provided via airbyte.yaml/airbyte.toml. Update the docstring to reflect the new config sources.
TEMP_FILE_CLEANUP = _SETTINGS.temp_file_cleanup
"""Whether to clean up temporary files after use.
Values use `AIRBYTE_TEMP_FILE_CLEANUP`, then `temp_file_cleanup` from `airbyte.yaml` or
airbyte/constants.py:76
- Docstring for DEFAULT_PROJECT_DIR still says it can only be overridden via the AIRBYTE_PROJECT_DIR env var, but this PR also allows configuration via airbyte.yaml/airbyte.toml. This is now misleading for users reading the constants docs.
This issue also appears in the following locations of the same file:
- line 102
- line 125
- line 136
"""Default project directory.
Values use `AIRBYTE_PROJECT_DIR`, then `project_dir` from `airbyte.yaml` or `airbyte.toml`, then
the current working directory.
| TEMP_DIR_OVERRIDE: Path | None = ( | ||
| Path(os.environ["AIRBYTE_TEMP_DIR"]) if os.getenv("AIRBYTE_TEMP_DIR") else None | ||
| ) | ||
| TEMP_DIR_OVERRIDE: Path | None = _SETTINGS.temp_dir |
| TEMP_DIR_OVERRIDE: Path | None = ( | ||
| Path(os.environ["AIRBYTE_TEMP_DIR"]) if os.getenv("AIRBYTE_TEMP_DIR") else None | ||
| ) | ||
| TEMP_DIR_OVERRIDE: Path | None = _SETTINGS.temp_dir |
There was a problem hiding this comment.
🚫 Not fixing. Re-raise of the same false positive on the newer commit — TEMP_DIR_OVERRIDE is imported and used in airbyte/_util/temp_files.py:33 and airbyte/_executors/util.py:301. The check appears to scope usage to constants.py, which is a module of constants that exist to be imported.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (5)
airbyte/settings.py:48
_parse_no_uv()claims to “preserve the inverted legacy AIRBYTE_NO_UV behavior”, but boolean inputs are returned unchanged. Because YAML/TOML can supply booleans, the legacy inversion is not preserved for file-basedno_uv: true/false. Either document this distinction explicitly or change the boolean handling to match the legacy string semantics.
def _parse_no_uv(value: object) -> bool:
"""Preserve the inverted legacy AIRBYTE_NO_UV behavior."""
if isinstance(value, bool):
return value
return str(value).lower() not in {"1", "true", "yes"}
airbyte/settings.py:41
_empty_path_to_none()currently treats any falsy value as “absent” (if not value:). When reading from YAML/TOML, this will silently convert invalid types likefalse/0intoNoneinstead of raising a validation error, which can mask misconfiguration. Limit the “empty -> None” behavior toNoneand the empty string to preserve the documented legacy behavior without accepting other falsy types.
def _empty_path_to_none(value: object) -> object:
"""Treat unset and empty path values as absent."""
if not value:
return None
return value
README.md:41
- README currently says “PyAirbyte defaults to uv” and that setting
AIRBYTE_NO_UV=truefalls back to pip, butNO_UVis implemented (and tested) with inverted legacy semantics whereAIRBYTE_NO_UV=true/1/yesenables uv and the default/unset case disables uv. Please update these lines so the docs match the actual behavior until the inversion is fixed in a follow-up.
By default, beginning with version `0.29.0`, PyAirbyte defaults to [`uv`](https://docs.astral.sh/uv) instead of `pip` for Python connector installation. Compared with `pip`, `uv` is much faster. It also provides the unique ability of specifying different versions of Python than PyAirbyte is using, and even Python versions which are not already pre-installed on the local workstation.
If you prefer to fall back to the prior `pip`-based installation methods, set the env var `AIRBYTE_NO_UV=true`.
tests/unit_tests/test_settings.py:78
- The precedence claim for
print_full_error_logs(env > file > default/CI-derived) isn’t currently asserted: this test sets a file value but never verifies thatAIRBYTE_PRINT_FULL_ERROR_LOGSoverrides it. Adding that assertion will also guard against anyenv_prefix/validation_aliasmisconfiguration for this setting.
settings = Settings()
assert settings.offline_mode is True
assert settings.cache_root == Path("/from-env")
assert settings.print_full_error_logs is False
airbyte/constants.py:183
- The
NO_UVdocstring describes non-inverted behavior ("AIRBYTE_NO_UV=true disables uv"), but the implementation (and new settings model) intentionally preserves the inverted legacy parsing where "true"/"1"/"yes" result inNO_UV == False(uv enabled). This is user-facing documentation and should reflect the actual semantics to avoid misconfiguration.
This value is determined by the `AIRBYTE_NO_UV` environment variable. When `AIRBYTE_NO_UV`
is set to "1", "true", or "yes", uv will be disabled and pip will be used instead.
If the variable is not set or set to any other value, uv will be used by default.
This provides a safe fallback mechanism for environments where uv is not available
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in commit 4507d76 in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in commit 4507d76 in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in commit 4507d76 in the Show a code coverage summary of the most impacted files.
Updated |
Summary
Requested by AJ (Aaron ("AJ") Steers (@aaronsteers)) as the PyAirbyte counterpart to airbytehq/airbyte-ops-mcp#1213: replace hand-rolled
os.getenvparsing with a typedpydantic-settingsmodel, and get optional file-based config for free.Scope is deliberately narrow — only the eight non-secret runtime settings that
airbyte/constants.pyreads at import time:AIRBYTE_PROJECT_DIR,AIRBYTE_INSTALL_DIR,AIRBYTE_CACHE_ROOT,AIRBYTE_TEMP_DIR,AIRBYTE_TEMP_FILE_CLEANUP,AIRBYTE_OFFLINE_MODE,AIRBYTE_PRINT_FULL_ERROR_LOGS,AIRBYTE_NO_UV. Nothing inairbyte/secrets/,airbyte/cloud/,airbyte/mcp/, or telemetry is touched: credential-shaped values keep resolving throughget_secret()/ the secrets subsystem, and a settings model must not shadow that.New
airbyte/settings.py:constants.pyconstructs oneSettings()at import and each public constant derives from it, so the existing import-time-snapshot semantics are unchanged — several tests andconftest.pydepend on those constants being frozen at import, so nothing here became lazy or dynamic.User-visible addition: an optional
airbyte.yaml/airbyte.tomlin the working directory. Precedence is env var > config file > default, verified empirically; a missing file is tolerated (the native sources checkPath.is_file()).Behavior preserved exactly
This is a refactor, so the quirks were kept rather than fixed:
_str_to_boolis not pydantic's bool parser —""/0/false/f/no/n/offare false and anything else is true, soAIRBYTE_OFFLINE_MODE=yesstill works. It moves tosettings.pyas aBeforeValidator.NO_UVis inverted relative to its name: it isTrueunlessAIRBYTE_NO_UVis1/true/yes, i.e. it defaults toTrue, which contradicts its own docstring. Preserved as-is and pinned with a characterization test — worth a separate look, not a drive-by fix here.TEMP_DIR_OVERRIDEisNonewhen the var is unset or empty.AIRBYTE_PRINT_FULL_ERROR_LOGSstill defaults from the unprefixedCIvar (env_prefixcan't express that, hence the explicitdefault_factory).DEFAULT_INSTALL_DIRstill chains off the resolvedDEFAULT_PROJECT_DIR,DEFAULT_CACHE_ROOToff<project>/.cache, and_try_create_dir_if_missingkeeps its directory-creation and warning behavior inconstants.py.airbyte/logs.pykeeps its own identical private_str_to_boolcopy; de-duplicating it is out of scope here.Test plan
New
tests/unit_tests/test_settings.pycovers missing config files, YAML and TOML loading, env-over-file precedence, the_str_to_booltruthiness table, theCI-derived default, theNO_UVinversion, and empty/non-empty temp dir values.uv run pytest -q tests/unit_tests/(456 passed, 1 skipped),uv run ruff check .,uv run ruff format --check .,uv run pyrefly check(0 errors),uv run deptry . --config pyproject.toml,uv lock --check.Link to Devin session: https://app.devin.ai/sessions/9b54bbbb80a945d99ce92c8940d41503
Summary by CodeRabbit
New Features
airbyte.yamlandairbyte.tomlruntime configuration support.Documentation