fix(config): use uv by default and honor AIRBYTE_NO_UV - #1099
fix(config): use uv by default and honor AIRBYTE_NO_UV#1099Aaron ("AJ") Steers (aaronsteers) merged 3 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/1785969891-fix-no-uv-inversion' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1785969891-fix-no-uv-inversion'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. |
📝 WalkthroughWalkthrough
ChangesNO_UV parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes the AIRBYTE_NO_UV environment-variable mapping so that uv is used by default (and pip is only used when opting out), aligning runtime behavior with the constant name/docstring and existing consumer logic.
Changes:
- Correct
NO_UVto beTrueonly whenAIRBYTE_NO_UVis explicitly set to a truthy value (1/true/yes), making uv the default. - Update the
NO_UVdocstring to describe the corrected behavior. - Add a subprocess-based unit test to validate the import-time env-var mapping across common values and casing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
airbyte/constants.py |
Fixes AIRBYTE_NO_UV → NO_UV parsing so uv is default and the opt-out flag behaves as named. |
tests/unit_tests/test_constants.py |
Adds coverage for the corrected env-var mapping using a subprocess to account for import-time evaluation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
airbyte/constants.py:190
- The final sentence implies that setting
AIRBYTE_NO_UVto any value opts out of uv, but the implementation only disables uv for the explicit truthy values {"1","true","yes"}. Tweaking the wording avoids ambiguity for users who might setAIRBYTE_NO_UV=0/falseand expect pip.
If the variable is not set or set to any other value, uv will be used by default. Set this
variable to opt out of uv and use pip instead.
Runtime verification — real connector installs confirm the flipVerified at the real subprocess level (not unit assertions): actually installed
Tests 1 and 3 use identical inputs and differ only in which tree is imported, so the opposite package manager is direct proof this one line is what flipped the behavior. Every case used a pristine The uv-built venv is actually usableReusing the venv created by So defaulting to uv does not produce a broken venv.
|
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in commit c3bd1c0 in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in commit c3bd1c0 in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in commit c3bd1c0 in the Show a code coverage summary of the most impacted files.
Updated |
Summary
AIRBYTE_NO_UVhas been wired backwards since a2b051e ("feat: replace pip with uv"), with the practical result that PyAirbyte has been installing connectors with pip, not uv, in the default configuration — the opposite of that PR's stated goal and of the constant's own docstring.The mapping is off by one
not:Every consumer already reads the constant the natural way (
airbyte/_executors/python.py,airbyte/validate.py):so
NO_UV = Trueby default meant theuvprefix was never applied and the venv/install fell back tosys.executable -m venvandpip. No consumer logic changes here; only the environment-variable mapping does.Behavior table:
AIRBYTE_NO_UV1/true/yes0/false/ anything elseuv>=0.5.0,<0.9.0is a direct runtime dependency, so uv is always present and the new default cannot fail for lack of the binary.Note for reviewers:
tests/conftest.pysetsAIRBYTE_NO_UV=1for itsuse_uv=Falseparametrization. Under the old mapping that case was silently exercising uv, and itsuse_uv=Truecounterpart was exercising pip — the two connector-install paths were swapped. This fix makes both match their names, so it also restores the intended test coverage rather than just flipping a default.Test plan
tests/unit_tests/test_constants.pypins the corrected mapping across unset,1,true,TRUE,YeS,0,false,no, and an arbitrary value. Each case runs in a subprocess becauseNO_UVis evaluated at import time and cannot be re-read by monkeypatching the environment in-process.Local:
uv run pytest -q tests/unit_tests/(445 passed, 1 skipped),ruff check,ruff format --check,pyrefly checkall clean.Related
Conflicts textually with #1098, which rewrites the same line to load from
pydantic-settingsand pins the old inverted behavior in a characterization test. Whichever lands second needs a rebase; if this one lands first, that characterization test should be updated to assert the corrected mapping.Link to Devin session: https://app.devin.ai/sessions/9b54bbbb80a945d99ce92c8940d41503
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit
Bug Fixes
AIRBYTE_NO_UVsetting so recognized truthy values consistently disable uv and use pip.Tests
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.