-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 base implementation #33409
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
@alafanechere To hook this into CI, I had to extend the global airbyte python gradle plugin a bit to deal with pyproject.toml files. As that's not my strongest area, could you take a look whether it makes sense the way I did it? |
One thing I'm not sure about when invoking connectors: Right now it works like this:
As we are dealing with reasonably well behaved python scripts here, they respect the sigterm and exit pretty quickly. However that means that they might now have the time to clean up after themselves. We could just wait for the process to terminate on its own - that would take a bit longer, but work for most cases, except for the "peek" method, because it isn't interested in all records but just the first few. So I can see some options here:
I tend towards the first one - I cancel python processes via ctrl+c in my daily workflows all the time, and I don't know of any "real" problems this would cause. If I'm missing something here, I would say we should go with the last option as otherwise it would be a weird in-between. |
@flash1293 could you please share your CI needs? We automate this already in the ci for airbyte-ci test themselves and a couple of other packages. I'll try to push a small commit to show you how it works in practice. In any case I suggest you nest your existing Edit: commit pushed |
Sounds good @alafanechere - you are right, I just want to run the tests for now. I will use your approach 👍 |
…into flash1293/airbyte-lib
Co-authored-by: Aaron ("AJ") Steers <aj@airbyte.io>
…into flash1293/airbyte-lib
@alafanechere I want to make sure the types are sound - what's the best way of running mypy on this project as part of the pipeline? Locally, this can be done via |
@aaronsteers Refactored as discussed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to go!
I did another pass and (just for context) linked a few places where subsequent PRs may slightly tweak existing behavior.
@@ -0,0 +1 @@ | |||
.venv* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that we have to move it, but I'd also be fine with adding to the root .gitignore
:
.venv
venv
airbyte-lib/airbyte_lib/cache.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I think we might refactor this. But no reason to block at this early stage.
# TODO this is a temporary install path that will be replaced with a proper package name once they are published. At this point we are also using the version | ||
package_to_install = f"../airbyte-integrations/connectors/{self.metadata.name}" | ||
self._run_subprocess_and_raise_on_failure([pip_path, "install", "-e", package_to_install]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an updated version of this in a new PR:
installed_version = self._get_installed_version() | ||
if installed_version != self.target_version: | ||
# If the version doesn't match, reinstall | ||
self.install() | ||
|
||
# Check the version again | ||
version_after_install = self._get_installed_version() | ||
if version_after_install != self.target_version: | ||
raise Exception( | ||
f"Failed to install connector {self.metadata.name} version {self.target_version}. Installed version is {version_after_install}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow-on PR, I suggest a flag to make version verification an opt-in behavior, where version would not be checked by default, but could be checked/auto-fixed as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will get less relevant once we publish properly to pypi, but I can see how the current state might be too restrictive or take too much control from the user.
What some tools are doing is to log a warning if the version doesn't match, but don't fail right away. Seems suitable here, but nothing we can't adjust before the first release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What some tools are doing is to log a warning if the version doesn't match, but don't fail right away. Seems suitable here, but nothing we can't adjust before the first release.
Totally! Yeah, I think a warning is a nice balance - gives the user information but doesn't necessary cause direct failure. In that case, we could always check version, and the "opt in" part would be whether to hard-fail, rather than warn. I can imagine a hard-failure might be preferred in specific instances where we think we are explicitly requesting a particular version number.
Thanks for the review, merging for now and we'll address these points on subsequent PRs |
Co-authored-by: alafanechere <augustin.lafanechere@gmail.com> Co-authored-by: Aaron ("AJ") Steers <aj@airbyte.io>
Co-authored-by: alafanechere <augustin.lafanechere@gmail.com> Co-authored-by: Aaron ("AJ") Steers <aj@airbyte.io>
What
Introduces the
airbyte-lib
package to run Python connectors within Python:Very first iteration of airbyte-lib.
Supported parts:
Missing: