fix(executors): discover connector console script from entry points#1071
Conversation
VenvExecutor no longer assumes the installed CLI name matches the connector name. When they differ, resolve the executable via console_scripts metadata instead of triggering a useless reinstall loop. Closes airbytehq#290 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note 📝 PR Converted to Draft More info...Thank you for creating this PR. As a policy to protect our engineers' time, Airbyte requires all PRs to be created first in draft status. Your PR has been automatically converted to draft status in respect for this policy. As soon as your PR is ready for formal review, you can proceed to convert the PR to "ready for review" status by clicking the "Ready for review" button at the bottom of the PR page. To skip draft status in future PRs, please include |
👋 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@sivharinair2001/fix-issue-290-executable-discovery' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@sivharinair2001/fix-issue-290-executable-discovery'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. |
📝 WalkthroughWalkthroughVenvExecutor now discovers a connector’s installed console-script name from package metadata, caches and resets that value across installation lifecycle events, and uses it for executable paths and installation validation. A mismatched-name fixture, regression test, and reproduction script verify the behavior. ChangesConsole Script Resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VenvExecutor
participant PackageMetadata
participant VirtualEnvironment
VenvExecutor->>PackageMetadata: discover installed console scripts
PackageMetadata-->>VenvExecutor: return wrong-script-name
VenvExecutor->>VirtualEnvironment: compute resolved executable path
VenvExecutor->>VirtualEnvironment: validate resolved executable
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
airbyte/_executors/python.py (3)
358-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe reinstall log message still references
self.nameinstead of the resolved script name — could this be misleading?Since the entire purpose of this PR is that the executable name may differ from
self.name, the log messagef"at {get_bin_dir(self._get_venv_path()) / self.name!s}"will show the wrong path when the names differ. Would it be clearer to show the actual bin directory or the resolved name, wdyt?♻️ Proposed fix
print( "Connector executable not found within the virtual environment " - f"at {get_bin_dir(self._get_venv_path()) / self.name!s}.\nReinstalling...", + f"in {get_bin_dir(self._get_venv_path())!s}.\nReinstalling...", file=sys.stderr, )🤖 Prompt for 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. In `@airbyte/_executors/python.py` around lines 358 - 362, Update the reinstall warning in the relevant executor logic to reference the resolved executable/script name rather than self.name. Use the same resolved-name variable used when locating the connector executable, while retaining the bin directory context in the displayed path.
104-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
elif len(entry_points) == 1branch is redundant — could we simplify?Both the
elif len(entry_points) == 1andelif entry_pointsbranches printentry_points[0].name, so the first branch is dead code. The single-entry-point case is already covered by theelif entry_pointsfallback. Would you consider collapsing these into a single branch, wdyt?♻️ Proposed simplification
if connector_name in {{ep.name for ep in entry_points}}: print(connector_name) -elif len(entry_points) == 1: - print(entry_points[0].name) elif entry_points: print(entry_points[0].name) else: print("")🤖 Prompt for 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. In `@airbyte/_executors/python.py` around lines 104 - 107, Remove the redundant `elif len(entry_points) == 1` branch and retain a single `elif entry_points` branch that prints `entry_points[0].name`, preserving the existing behavior for one or more entry points.
369-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShould
discovered_console_scriptsin the error context be more informative?When this error is reached,
_resolve_console_script_name()has already returnedNonetwice (before and after reinstall), meaning both the default path check and_discover_console_script_name()failed. Calling_discover_console_script_name()again at line 375 will likely returnNoneagain, making thediscovered_console_scriptscontext field alwaysNone. Would it be worth listing all console scripts in the venv's bin directory instead, or adjusting the field name to reflect that it may beNone, wdyt?🤖 Prompt for 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. In `@airbyte/_executors/python.py` around lines 369 - 377, Make the installation error context informative by avoiding the redundant _discover_console_script_name() call after _resolve_console_script_name() has already failed. Update the discovered_console_scripts field to capture and list the console scripts present in the virtual environment’s bin directory, or rename it to accurately describe the nullable resolution result, using the relevant helper methods in the connector installation flow.
🤖 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.
Nitpick comments:
In `@airbyte/_executors/python.py`:
- Around line 358-362: Update the reinstall warning in the relevant executor
logic to reference the resolved executable/script name rather than self.name.
Use the same resolved-name variable used when locating the connector executable,
while retaining the bin directory context in the displayed path.
- Around line 104-107: Remove the redundant `elif len(entry_points) == 1` branch
and retain a single `elif entry_points` branch that prints
`entry_points[0].name`, preserving the existing behavior for one or more entry
points.
- Around line 369-377: Make the installation error context informative by
avoiding the redundant _discover_console_script_name() call after
_resolve_console_script_name() has already failed. Update the
discovered_console_scripts field to capture and list the console scripts present
in the virtual environment’s bin directory, or rename it to accurately describe
the nullable resolution result, using the relevant helper methods in the
connector installation flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb535d68-b380-484c-b62d-e790b3c0e899
📒 Files selected for processing (6)
airbyte/_executors/python.pyscripts/reproduce_issue_290.pytests/integration_tests/fixtures/source-wrong-exe/setup.pytests/integration_tests/fixtures/source-wrong-exe/source_wrong_exe/__init__.pytests/integration_tests/fixtures/source-wrong-exe/source_wrong_exe/run.pytests/unit_tests/test_issue_290_wrong_executable.py
2 participants
VenvExecutor no longer assumes the installed CLI name matches the connector name. When they differ, resolve the executable via console_scripts metadata instead of triggering a useless reinstall loop.
Closes #290
Summary by CodeRabbit
Bug Fixes
Tests