CDK MyPy #3175

davinchia · 2021-05-03T09:34:43Z

What

Before we write tests, run MyPy on the base_python module in order to ensure consistent interfaces/types throughout the module.

Have added comments where-ever necessary to explain the error.

Since there the jsonschema module doesn't have a typeshed definition, I've instructed MyPy to ignore it.

How

mypy airbyte_cdk/base_python shows no errors.

➜  python git:(davinchia/cdk-mypy) ✗ mypy airbyte_cdk/base_python
Success: no issues found in 22 source files

Note, I attempted to fix the singer module and didn't get far since the interfaces have changed quite drastically. We should look at this during our refactor.

…ython/integration.py. Fix base_python/source.py. Fix singer/source.py.

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/auth/oauth.py

davinchia · 2021-05-03T09:53:41Z

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/auth/oauth.py

        """ Override to define additional parameters """
-        payload = {
+        payload: MutableMapping[str, Any] = {


had to define this as the return type is a Mapping and MyPy threw and error as we are modifying this struct before returning it.

davinchia · 2021-05-03T09:54:35Z

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/core.py

@@ -79,7 +79,7 @@ def as_airbyte_stream(self) -> AirbyteStream:

        if self.supports_incremental:
            stream.source_defined_cursor = self.source_defined_cursor
-            stream.supported_sync_modes.append(SyncMode.incremental)
+            stream.supported_sync_modes.append(SyncMode.incremental) # type: ignore


this is ignored as it's I'm fairly certain this is assigned. MyPy isn't detecting and assumes the field is None for some reason.

davinchia · 2021-05-03T09:55:55Z

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/http.py

        cursor_field: List[str] = None,
+        stream_slice: Mapping[str, Any] = None,


this previously did not match the Stream interface's read_records method

davinchia · 2021-05-03T09:57:45Z

airbyte-cdk/python/airbyte_cdk/base_python/integration.py

        return ConnectorSpecification.parse_obj(json.loads(raw_spec))

-    def check(self, logger: AirbyteLogger, config: json) -> AirbyteConnectionStatus:
+    def check(self, logger: AirbyteLogger, config: Mapping[str, Any]) -> AirbyteConnectionStatus:


These function signatures previously did not match the classes implementing them. This now matches.

davinchia · 2021-05-03T09:58:06Z

airbyte-cdk/python/airbyte_cdk/base_python/source.py

@@ -46,7 +46,7 @@
 class BaseSource(Source):
    """Base source that designed to work with clients derived from BaseClient"""

-    client_class: Type[BaseClient] = None
+    client_class: Type[BaseClient]


the class type hint shouldn't be assigned to

davinchia · 2021-05-03T09:58:47Z

airbyte-cdk/python/airbyte_cdk/singer/source.py

@@ -116,7 +116,7 @@ def discover(self, logger: AirbyteLogger, config_container) -> AirbyteCatalog:

    def read(
        self, logger: AirbyteLogger, config_container: ConfigContainer, catalog_path: str, state_path: str = None
-    ) -> Generator[AirbyteMessage, None, None]:
+    ) -> Iterator[AirbyteMessage, None, None]:


To match the Source interface.

can we make it simpler by changing both to Iterable[AirbyteMessage]?

yeap. this is from the existing interface type.

davinchia · 2021-05-03T10:26:41Z

/test connector=source-appstore-singer

🕑 source-appstore-singer https://github.com/airbytehq/airbyte/actions/runs/806507873
✅ source-appstore-singer https://github.com/airbytehq/airbyte/actions/runs/806507873

ChristopheDuong

mypy does really help going through the code, Nice!

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/auth/oauth.py

sherifnada

nice! I think there is a breaking change in http.py.

Also, where is MyPy installed? Shouldn't it be in setup.py?

airbyte-cdk/python/README.md

sherifnada · 2021-05-03T16:49:35Z

airbyte-cdk/python/airbyte_cdk/base_python/cdk/abstract_source.py

-        args = {"sync_mode": SyncMode.full_refresh, "cursor_field": configured_stream.cursor_field}
-        for slices in stream_instance.stream_slices(**args):
-            for record in stream_instance.read_records(stream_slice=slices, **args):
+        for slices in stream_instance.stream_slices(sync_mode=SyncMode.full_refresh, cursor_field=configured_stream.cursor_field):


why make this change?

matter of taste, but I find it is more readable like this:

records = stream_instance.read_records(stream_slice=slices, sync_mode=SyncMode.full_refresh, cursor_field=configured_stream.cursor_field) for record in records:

➜ python git:(davinchia/cdk-mypy) ✗ mypy airbyte_cdk/base_python airbyte_cdk/base_python/cdk/abstract_source.py:170: error: Argument 1 to "stream_slices" of "Stream" has incompatible type "**Dict[str, Union[SyncMode, List[str], None]]"; expected "SyncMode" airbyte_cdk/base_python/cdk/abstract_source.py:170: error: Argument 1 to "stream_slices" of "Stream" has incompatible type "**Dict[str, Union[SyncMode, List[str], None]]"; expected "Optional[List[str]]" airbyte_cdk/base_python/cdk/abstract_source.py:170: error: Argument 1 to "stream_slices" of "Stream" has incompatible type "**Dict[str, Union[SyncMode, List[str], None]]"; expected "Optional[Mapping[str, Any]]" Found 3 errors in 1 file (checked 22 source files)

MyPy doesn't like kwargs, since it can't be sure that the entries actually exists and are bound with the right type.

seems reasonable to make it here

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/auth/oauth.py

sherifnada · 2021-05-03T16:53:06Z

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/http.py

-                headers=dict(self.request_headers(**args), **self.authenticator.get_auth_header()),
-                params=self.request_params(**args),
-                json=self.request_body_json(**args),
+                path=self.path(args),


This seems incorrect. **args and args are different. The former denests keyword args from a dictionary, the latter passes a dict as an argument.

Okay I figured this out - the header and request_body json functions do not accept an Optional with their first parameter, which is causing this error:

➜ python git:(davinchia/cdk-mypy) ✗ mypy airbyte_cdk/base_python airbyte_cdk/base_python/cdk/streams/http.py:218: error: Argument 1 to "request_params" of "HttpStream" has incompatible type "**Dict[str, Optional[Mapping[str, Any]]]"; expected "Mapping[str, Any]" airbyte_cdk/base_python/cdk/streams/http.py:219: error: Argument 1 to "request_body_json" of "HttpStream" has incompatible type "**Dict[str, Optional[Mapping[str, Any]]]"; expected "Mapping[str, Any]" Found 2 errors in 1 file (checked 22 source files)

I think the function signature should continue being specific (instead of accepting Optional), so I'm going to break out the arguments. What do you think?

airbyte-cdk/python/airbyte_cdk/base_python/integration.py

sherifnada · 2021-05-03T16:54:14Z

airbyte-cdk/python/airbyte_cdk/base_python/integration.py

        return ConnectorSpecification.parse_obj(json.loads(raw_spec))

-    def check(self, logger: AirbyteLogger, config: json) -> AirbyteConnectionStatus:
+    def check(self, logger: AirbyteLogger, config: Mapping[str, Any]) -> AirbyteConnectionStatus:


sherifnada · 2021-05-03T16:54:37Z

airbyte-cdk/python/airbyte_cdk/singer/source.py

@@ -116,7 +116,7 @@ def discover(self, logger: AirbyteLogger, config_container) -> AirbyteCatalog:

    def read(
        self, logger: AirbyteLogger, config_container: ConfigContainer, catalog_path: str, state_path: str = None
-    ) -> Generator[AirbyteMessage, None, None]:
+    ) -> Iterator[AirbyteMessage, None, None]:


can we make it simpler by changing both to Iterable[AirbyteMessage]?

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

… into davinchia/cdk-mypy

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/http.py

sherifnada

there is one more breaking change in http.py but otherwise lgtm

davinchia added 5 commits May 3, 2021 13:52

Checkpoint: Add .mypy.ini file. Fix base_python/client.py. Fix base_p…

1fb3972

…ython/integration.py. Fix base_python/source.py. Fix singer/source.py.

Checkpoint: Fix oauth.py.

b7a9ccf

Fix core.py.

f7d5bb2

Checkpoint: Fix abstract_source.py.

f3ca852

Checkpoint: Fix http.py.

b87c770

davinchia commented May 3, 2021

View reviewed changes

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/auth/oauth.py Show resolved Hide resolved

davinchia commented May 3, 2021

View reviewed changes

Better comments.

05254a6

davinchia requested a review from sherifnada May 3, 2021 11:08

davinchia marked this pull request as ready for review May 3, 2021 11:18

auto-assign bot requested review from ChristopheDuong and jrhizor May 3, 2021 11:18

davinchia removed the request for review from jrhizor May 3, 2021 11:18

ChristopheDuong approved these changes May 3, 2021

View reviewed changes

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/auth/oauth.py Show resolved Hide resolved

sherifnada suggested changes May 3, 2021

View reviewed changes

sherifnada mentioned this pull request May 3, 2021

Use MyPy for static type checking in the CDK #3190

Closed

davinchia and others added 7 commits May 4, 2021 05:49

Update airbyte-cdk/python/README.md

8cbe43a

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

PR feedback.

fd88c75

Merge remote-tracking branch 'refs/remotes/origin/davinchia/cdk-mypy'…

6930de5

… into davinchia/cdk-mypy

Better http.py refactor.

5abf0ca

Use Mapping[str, Any] instead of object.

ae19935

Add MyPy to setup.py.

d1b0f38

Change to Iterable.

206d231

sherifnada reviewed May 3, 2021

View reviewed changes

airbyte-cdk/python/airbyte_cdk/base_python/cdk/streams/http.py Outdated Show resolved Hide resolved

sherifnada approved these changes May 4, 2021

View reviewed changes

davinchia changed the title ~~Davinchia/cdk mypy~~ CDK MyPy May 4, 2021

davinchia added 3 commits May 4, 2021 10:29

Make sure to denest.

66ad253

Respond to PR feedback.

3251bb8

Merge branch 'master' into davinchia/cdk-mypy

c737856

davinchia merged commit 72e7fe3 into master May 4, 2021

davinchia deleted the davinchia/cdk-mypy branch May 4, 2021 03:02

igrankova added connectors/sources-api connectors/source/appstore-singer labels Jan 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDK MyPy #3175

CDK MyPy #3175

davinchia commented May 3, 2021 •

edited

Loading

davinchia May 3, 2021 •

edited

Loading

davinchia May 3, 2021 •

edited

Loading

davinchia May 3, 2021 •

edited

Loading

sherifnada May 3, 2021

davinchia May 3, 2021

sherifnada May 3, 2021

davinchia May 3, 2021

davinchia May 3, 2021

sherifnada May 3, 2021

davinchia May 3, 2021

davinchia commented May 3, 2021 •

edited by github-actions bot

Loading

ChristopheDuong left a comment

sherifnada left a comment •

edited

Loading

sherifnada May 3, 2021

keu May 3, 2021

davinchia May 3, 2021

sherifnada May 3, 2021

sherifnada May 3, 2021

davinchia May 3, 2021

sherifnada May 3, 2021

sherifnada May 3, 2021

sherifnada left a comment

		cursor_field: List[str] = None,
		stream_slice: Mapping[str, Any] = None,

CDK MyPy #3175

CDK MyPy #3175

Conversation

davinchia commented May 3, 2021 • edited Loading

What

How

davinchia May 3, 2021 • edited Loading

Choose a reason for hiding this comment

davinchia May 3, 2021 • edited Loading

Choose a reason for hiding this comment

davinchia May 3, 2021 • edited Loading

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

davinchia commented May 3, 2021 • edited by github-actions bot Loading

ChristopheDuong left a comment

Choose a reason for hiding this comment

sherifnada 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

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

sherifnada left a comment

Choose a reason for hiding this comment

davinchia commented May 3, 2021 •

edited

Loading

davinchia May 3, 2021 •

edited

Loading

davinchia May 3, 2021 •

edited

Loading

davinchia May 3, 2021 •

edited

Loading

davinchia commented May 3, 2021 •

edited by github-actions bot

Loading

sherifnada left a comment •

edited

Loading