-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
AirbyteLib: More robust error handling, installation improvements #34572
Conversation
…le flag during install
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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 was unable to debug a new notebook I was creating for GitHub analytics. This PR addresses some of those challenges I ran into.
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.
Thanks for these changes, I agree with most of it, left some nit and one question.
@@ -185,7 +193,7 @@ class AirbyteConnectorError(AirbyteError): | |||
|
|||
|
|||
class AirbyteConnectorNotFoundError(AirbyteConnectorError): | |||
"""Connector not found.""" | |||
"""Connector name not found in registry.""" |
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.
Looking at the code, by my understanding AirbyteConnectorNotFoundError
means the connector is not found locally, and AirbyteConnectorNotRegisteredError
means the connector is not found in the registry, but this docstring indicates otherwise.
Could you clarify?
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.
Good catch! This was a mistake. I've renamed the error to AirbyteConnectorExecutableNotFoundError
so it is more explicit.
airbyte-lib/airbyte_lib/registry.py
Outdated
@@ -12,47 +14,81 @@ | |||
from airbyte_lib.version import get_version | |||
|
|||
|
|||
__cache: dict[str, ConnectorMetadata] | None = None | |||
_cache_lock = threading.Lock() |
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 seems like this lock is never actually aquired? Am I missing something?
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.
Good catch. I had it in an earlier version when debugging a race condition, then refactored so it wasn't needed.
(Now removed.)
self, | ||
config: dict[str, Any], | ||
*, | ||
validate: bool = False, |
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's the advantage of deferring the check? I thought of it being quite nice as it will tell you as early as possible if your config won't work, instead of waiting for actually invoking.
What's the workflow you had in mind here?
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.
Discussed in DM.
This does a few things:
-e
(editable) flag from install. Users can still get an editable install by using-e ./path/to/my-connector
in pip_url. (This was done because -e seemed to fail when used with installations that were not from local paths.)