Feat: Add install_root arg for get_source(), allows users to override virtualenv base path

[Tang Quoc Thai (tqtensor)]

As I want to run PyAirbyte on AWS Lambda, I encountered this error because on Lambda there is one place folder that the program can write data into is /tmp/

I searched through the current source code of PyAirbyte and found out that there is an argument named install_root to specify where the new .venv-source-name folder is.

So, I made this change to install the source connector virtual environment to the tmp folder.

Exception:

airbyte.exceptions.AirbyteSubprocessFailedError: AirbyteSubprocessFailedError: Subprocess failed.
--
Run Args: ['/var/lang/bin/python3.10', '-m', 'venv', '/var/task/.venv-source-stripe']
Exit Code: 1
Log output:
Error: [Errno 30] Read-only file system: '/var/task/.venv-source-stripe'

Summary by CodeRabbit

• New Features
  • Added a new parameter install_root to functions for specifying the root directory when creating a virtual environment for connectors.

[Aaron ("AJ") Steers (aaronsteers)]

/fix-pr

[Aaron ("AJ") Steers (aaronsteers)]

Tang Quoc Thai (@tqtensor) - This looks great to me. Thanks for this contribution.

Can you help make the linter happy again by adding a noqa for PLR0913 on the _get_source( declaration line?

[Tang Quoc Thai (tqtensor)]

Hi Aaron ("AJ") Steers (@aaronsteers), that issue is raised because we add too many arguments; one workaround is using kwargs or making a data class for these arguments to abstract them. I prefer to go with the data class or pydantic.

[Aaron ("AJ") Steers (aaronsteers)]

Hi Aaron ("AJ") Steers (@aaronsteers), that issue is raised because we add too many arguments; one workaround is using kwargs or making a data class for these arguments to abstract them. I prefer to go with the data class or pydantic.

Tang Quoc Thai (@tqtensor) I appreciate the attention to proper refactoring. However, I would rather not add additional refactoring at this time. This is in part because we have at least one other PR that is targeting this same code location; I'd prefer to avoid adding merge conflicts to resolve in related PRs, and to reduce scope/effort to get this merged. I think the simplest and safest path for now is just to add another lint ignore for now.

Does that sound okay for now?

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 336ec6c and 01b8864.

Files selected for processing (1)

• airbyte/sources/util.py (7 hunks)

Files skipped from review as they are similar to previous changes (1)

• airbyte/sources/util.py

[Tang Quoc Thai (tqtensor)]

Hi Aaron ("AJ") Steers (@aaronsteers), that issue is raised because we add too many arguments; one workaround is using kwargs or making a data class for these arguments to abstract them. I prefer to go with the data class or pydantic.

Tang Quoc Thai (@tqtensor) I appreciate the attention to proper refactoring. However, I would rather not add additional refactoring at this time. This is in part because we have at least one other PR that is targeting this same code location; I'd prefer to avoid adding merge conflicts to resolve in related PRs, and to reduce scope/effort to get this merged. I think the simplest and safest path for now is just to add another lint ignore for now.

Does that sound okay for now?

Aaron ("AJ") Steers (@aaronsteers), Yes, it is fine as you have a plan or ongoing work to refactor that, so I did add the #noqa for PLR0913.

[Aaron ("AJ") Steers (aaronsteers)]

Looks great! Thanks again for this contribution! 🚀

[Aaron ("AJ") Steers (aaronsteers)]

/test-pr

PR test job started... Check job output.

❌ Tests failed.
PR test job started... Check job output.

❌ Tests failed.