fix(airbyte-cdk): unable to create custom retriever #198
Conversation
aldogonzalez8
changed the title
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Wdyt about the related PRs? Do you think they could provide additional context for your changes? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1899-1899: Double-check optional transformations parameter.Making
transformationsoptional simplifies usage. Nice addition! Would you consider adding a docstring clarifying the default empty list? wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting using Ruff formatter
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
2631-2633: Add a docstring or comment for the custom retriever?Your
MyCustomRetrieverclass inheritsSimpleRetrieverbut has no docstring. Would you add a quick description for clarity? wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting using Ruff formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(3 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py
[warning] File needs formatting using Ruff formatter
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[warning] File needs formatting using Ruff formatter
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1515-1515: Consider verifying default decoder usage.Setting
decoder=JsonDecoder(parameters={})as the default is handy. However, if a user wants no decoding, do you plan to support passingNoneor a different decoder? wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting using Ruff formatter
1931-1931: Good fallback to empty list for transformations.This effectively prevents errors from passing
None. Looks great to me!🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting using Ruff formatter
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
2635-2665: Solid test for custom retriever creation.This covers instantiating a custom retriever in a
DeclarativeStream. Everything looks in order. Great job!🧰 Tools
🪛 GitHub Actions: Linters
[warning] File needs formatting using Ruff formatter
|
/format-fix |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (2)
2637-2639: Consider adding a docstring or comment to describe the purpose of this subclass.
Even though it's a simple subclass, it might help future developers if you provide a quick explanation of this custom retriever. wdyt?
2641-2671: Add an explicit assertion to ensureMyCustomRetrieveris properly tested.
Would you consider adding a small test that verifies any overridden methods or states if added in the future? For example, by checking the retriever’s internal attributes or behavior? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(3 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Outdated
Show resolved
Hide resolved
… from create_record_selector method
There was a problem hiding this comment.
source-linkedinads has a custom retriever. Was it broken? Is there anything to do with this one?
@maxi297 Ok, it took me a few annoying minutes of debugging but I think I found why this works fine for linkedin-ads, I can see every Custom Retriever is paried with a Custom Requester
So the custom retriever is not tied to a "regular" requester like HttpRequester, so they don't depend on create_httprequester but on Linkedin ads:
On the other side, this PR tries to fix the scenario of creating a CustomRetriever with no-custom requester like HttpRequester that will use create_http_requester. Regularly I think this works fine as create_simple_retriever will create a decoder if missing. |
|
@maxi297 Also doing a dummy experiment if I try to create a CustomRetriever with an HttpRequester in linkedin-ads I see the error I observed when working with google-sheets
|
* main: fix(airbyte-cdk): unable to create custom retriever (airbytehq#198) feat(low-code): added keys replace transformation (airbytehq#183) feat: add `min` macros (airbytehq#203) fix(low-code cdk pagination): Fix the offset strategy so that it resets back to 0 when a stream is an incremental data feed (airbytehq#202)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
create_http_requester.create_record_selector.What?
I was encountering errors like the following when creating a Custom Retriever:
From what I can see, decoder can be optional here can is default to JsonDecoder here.
For transformations also we set it as optional here.