Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

airbyte-lib: Add path executor #33600

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

flash1293
Copy link
Contributor

This PR adds a path executor which allows to bypass the venv if the connector is installed locally and available via a cli script.

This behavior can be enabled via setting the use_venv param of get_connector to false:

source = ab.get_connector("source-apify-dataset") # use a venv
source2 = ab.get_connector("source-apify-dataset", use_venv=False) # use `source-apify-dataset` from the path, fails if it doesn't exist

Copy link

vercel bot commented Dec 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2023 7:00am

@stephane-airbyte
Copy link
Contributor

@flash1293 I think you ran into a bad merge. You'll have to close this PR, rebase your branch on master and recreate a PR (if you just rebase your branch, everyone currently on this PR will be ping everytime something happens here)

@aaronsteers aaronsteers removed request for a team December 19, 2023 06:53
@aaronsteers
Copy link
Collaborator

@flash1293 - When I tried to use Graphite on my own PRs (based on the same base as this one), it force pushed all 3 branches, and may have been the cause of this one getting into a bad state.

I updated the base from master and this one from the base branch, and it looks okay now.

Copy link
Collaborator

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question inline, but non-blocking.

Approved for merge. 👍

Comment on lines +19 to +26
def get_connector(name: str, version: str = "latest", config: Optional[Dict[str, Any]] = None, use_venv: bool = True):
"""
Get a connector by name and version.
:param name: connector name
:param version: connector version - if not provided, the most recent version will be used
:param config: connector config - if not provided, you need to set it later via the set_config method
:param use_venv: whether to use a virtual environment to run the connector. If False, the connector is expected to be available on the path (e.g. installed via pip). If True, the connector will be installed in a virtual environment.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read "use_venv" the first time to mean that "false" would still be managed, but would be installed by AirbyteLib without venv isolation. Wdyt of a name like "already_installed" or "needs_install" or similar to distinguish AirbyteLib-managed installations vs user-managed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about "auto_install"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged already, I will implement this on the base PR

@flash1293 flash1293 merged commit 9e003a9 into flash1293/airbyte-lib Dec 19, 2023
22 of 23 checks passed
@flash1293 flash1293 deleted the flash1293/other-executors-2 branch December 19, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants