Framework v0.1 #2634

sherifnada · 2021-03-29T07:55:02Z

closes #2647
closes #2651

Alpha version of the source framework.

Main additions:

New abstractions for sources, full refresh streams, incremental streams, HTTP streams
implements the Stripe API using the new framework. Compare this to the Singer implementation here: https://github.com/singer-io/tap-stripe/blob/master/tap_stripe/__init__.py

Current features of HTTP stream:

auth:
- API-token based auth
two kinds of out-of-the-box rate limiting: static and dynamic (determined at runtime potentially by inspecting response)
support for generating N requests per stream e.g: for making one request per day like in exchangeratesapi
support for arbitrary nesting of child streams. Just pass child=<stream> and everything (mostly) wires up correctly.
ability to customize all request parameters based on context (parent streams, current request context, etc..)

Reading Order

source_stripe/source.py for an example source (this could replace the singer Stripe source)
abstract_source.py for how things all come together
streams/core.py for the core stream interface
streams/http.py
auth/core.py and auth/simple.py for a basic idea of the oauth stuff
streams/rate_limiting.py
everything else

airbyte-integrations/bases/base-python/base_python/sdk/abstract_source.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/auth/oauth.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/rate_limiting.py

...ntegrations/connectors/source-stripe-singer/sample_files/fullrefresh_configured_catalog.json

airbyte-integrations/bases/base-python/base_python/sdk/streams/http.py

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

jrhizor

State handling is a bit confusing
The responsibilities of each method for updating/outputting doesn't feel obvious while reading through the code.

Typing
There are a lot of Dicts everywhere and it makes it hard to read and very comment-dependent to understand the code. I feel like a lot of this could be improved by aliasing (configs, different JSON-style dicts, etc).

Performance
How does the performance of this compare to the singer Stripe source? I'd like to make sure the out-of-the-box behavior is comparable.

airbyte-integrations/bases/base-python/base_python/sdk/streams/auth/simple.py

airbyte-integrations/connectors/source-oracle/README.md

airbyte-integrations/connectors/source-stripe/requirements.txt

airbyte-integrations/connectors/source-stripe/sample_files/full_catalog.json

airbyte-protocol/models/src/main/java/io/airbyte/protocol/models/Field.java

airbyte-integrations/bases/base-python/base_python/sdk/streams/http.py

jrhizor · 2021-04-01T19:01:35Z

airbyte-integrations/bases/base-python/base_python/sdk/streams/http.py

+        :param response:
+        :return: An iterable containing the parsed response
+        """
+        yield [response]


Feels strange to need to override these different methods when really we're trying to go from requests.Response -> Iterable[Dict].

Maybe we could use types to show how these are intended to chain together?

jrhizor · 2021-04-01T19:03:15Z

airbyte-integrations/bases/base-python/base_python/sdk/streams/http.py

+        """
+        return response.status_code == 429 or 500 <= response.status_code < 600
+
+    def backoff_time(self, response: requests.Response) -> Optional[float]:


Maybe this should specify behavior for a backoff instead of just allowing time? Like attempt number, response, and then allow calculating time? Then someone could do exp. backoff scaled their own way for example.

Also it feels like this should be a Duration equivalent or milliseconds instead of seconds-level.

Discussed in person. Sounds like this will move towards custom decorators/handlers instead of these function overrides.

fwiw this might be a lot cleaner if we can get this merged: litl/backoff#122

airbyte-integrations/bases/base-python/base_python/sdk/streams/rate_limiting.py

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

airbyte-integrations/bases/base-python/base_python/sdk/abstract_source.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

michel-tricot · 2021-04-02T05:16:27Z

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

+    def get_updated_state(self, current_state: Mapping[str, Any], latest_record: Mapping[str, Any]):
+        """
+        Inspect the latest record extracted from the data source and the current state object and return an updated state object.
+        It is safe to mutate the input state object and return it.


Why would you want to allow that? We shouldn't encourage that.

especially taking into account that Mapping is immutable (for modification there is MutableMapping)
also return type is missing.

the alternative is to copy the input state every time (or to create a new state object in this method) which seems not great for performance?

airbyte-integrations/bases/base-python/base_python/sdk/streams/http.py

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

cgardens

cool! added a few thoughts. i'm going to just leave a comment since you are not lacking for reviewers on this one!

airbyte-integrations/bases/base-python/base_python/sdk/abstract_source.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/auth/simple.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

cgardens · 2021-04-02T22:38:29Z

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

+
+class IncrementalStream(Stream, ABC):
+    @property
+    def cursor_field(self) -> Union[str, List[str]]:


cgardens · 2021-04-02T22:40:38Z

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

+    @property
+    def cursor_field(self) -> Union[str, List[str]]:
+        """
+        Override to return the name of the default cursor field used by this stream e.g: an API entity might always use created_at as the cursor field.


should this method be called default_cursor_field? it sounds like this is only populated if the source is setting the cursor field if i'm reading the comment correctly. or maybe call this method source_defined_cursor and then the method be low is just has_source_defined_cursor and default impl can just check if this method returns null / empty array

I think there is either cursor_field or not, what makes it default_cursor_field? If there is default_cursor_field then there should be actual/current_cursor_field ?
also what is the case when we need additional property source_defined_cursor?
isn't cursor_field self sufficient? i.e. it is either null/[] or str/[str]

cursor_field in the general case could be chosen by a user. For example when replicating data from a SQL database, you could choose to replicate based on created_at or updated_at. In this case, because the cursor is configurable by the user, source_defined_cursor would return False even though the cursor field is defined.

I think base abstraction should include this logic of setting custom cursor field

I think the name source_defined_cursor could be confusing, maybe something like cursor_field_overriden or has_custom_cursor_field?

If we have cursor_field and default_cursor_field, wouldn't source_defined_cursor implementation just compare if one is equal to another? Also I still don't see the case when the stream might need this information.

Added this

Agreed with you. Unfortunately currently this is at the protocol level so changing this is a heavy lift because it would change all connectors, the UI, and the core API. To make this better I defaulted all HTTP streams to True on this field so the user doesn't need to think about this (most HTTP sources have a static cursor field anyways unlike e.g: databases)

keu · 2021-04-03T10:21:56Z

airbyte-integrations/bases/base-python/base_python/sdk/streams/core.py

+    @property
+    def cursor_field(self) -> Union[str, List[str]]:
+        """
+        Override to return the name of the default cursor field used by this stream e.g: an API entity might always use created_at as the cursor field.


I think there is either cursor_field or not, what makes it default_cursor_field? If there is default_cursor_field then there should be actual/current_cursor_field ?
also what is the case when we need additional property source_defined_cursor?
isn't cursor_field self sufficient? i.e. it is either null/[] or str/[str]

airbyte-integrations/bases/base-python/base_python/sdk/abstract_source.py

airbyte-integrations/bases/base-python/base_python/sdk/streams/auth/token.py

michel-tricot · 2021-04-07T15:38:41Z

🥇

auto-assign bot requested review from cgardens and ChristopheDuong March 29, 2021 07:55

sherifnada marked this pull request as draft March 29, 2021 07:55

sherifnada changed the base branch from master to mt-integration-fmk March 29, 2021 07:55

sherifnada removed request for ChristopheDuong and cgardens March 29, 2021 07:58

sherifnada changed the title ~~wip sdk~~ wip santa Mar 31, 2021

sherifnada changed the title ~~wip santa~~ Framework v0.1 Mar 31, 2021

sherifnada marked this pull request as ready for review April 1, 2021 00:15

auto-assign bot requested review from cgardens and davinchia April 1, 2021 00:15

sherifnada requested review from michel-tricot, ChristopheDuong, jrhizor and keu April 1, 2021 00:15

keu suggested changes Apr 1, 2021

View reviewed changes

jrhizor suggested changes Apr 1, 2021

View reviewed changes

sherifnada mentioned this pull request Apr 1, 2021

CDK sources: use spec to validate config #2711

Closed

michel-tricot requested changes Apr 2, 2021

View reviewed changes

cgardens reviewed Apr 2, 2021

View reviewed changes

keu suggested changes Apr 3, 2021

View reviewed changes

This was referenced Apr 5, 2021

framework: handle server throttling and client throttling in HTTP Stream #2648

Closed

santa: HTTP Abstraction stress test #2647

Closed

framework: snakecase stream names from class names by default #2723

Closed

michel-tricot reviewed Apr 7, 2021

View reviewed changes

sherifnada added 2 commits April 7, 2021 00:08

save

d5d2cfc

nuclear option

272d4b9

sherifnada force-pushed the sherif/fork-magibyte branch from ff14bc2 to 272d4b9 Compare April 7, 2021 07:09

sherifnada changed the base branch from mt-integration-fmk to master April 7, 2021 07:09

sherifnada added 3 commits April 7, 2021 00:12

format

5583012

wut

abf3e1c

improve unit tests

c65c21c

sherifnada merged commit 94093f0 into master Apr 7, 2021

sherifnada deleted the sherif/fork-magibyte branch April 7, 2021 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework v0.1 #2634

Framework v0.1 #2634

sherifnada commented Mar 29, 2021 •

edited

Loading

jrhizor left a comment

jrhizor Apr 1, 2021

jrhizor Apr 1, 2021

jrhizor Apr 1, 2021

jrhizor Apr 6, 2021

sherifnada Apr 7, 2021

michel-tricot Apr 2, 2021

keu Apr 2, 2021

sherifnada Apr 6, 2021

cgardens left a comment •

edited

Loading

cgardens Apr 2, 2021

cgardens Apr 2, 2021

keu Apr 3, 2021 •

edited

Loading

sherifnada Apr 6, 2021

keu Apr 6, 2021 •

edited

Loading

sherifnada Apr 7, 2021

keu Apr 3, 2021 •

edited

Loading

michel-tricot commented Apr 7, 2021

Framework v0.1 #2634

Framework v0.1 #2634

Conversation

sherifnada commented Mar 29, 2021 • edited Loading

Reading Order

jrhizor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keu Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

michel-tricot commented Apr 7, 2021

sherifnada commented Mar 29, 2021 •

edited

Loading

cgardens left a comment •

edited

Loading

keu Apr 3, 2021 •

edited

Loading

keu Apr 6, 2021 •

edited

Loading

keu Apr 3, 2021 •

edited

Loading