Airbyte-ci: Add local cdk support #30461

bnchrch · 2023-09-15T02:07:16Z

Overview

This allows developers to build and test connectors using the local version of the cdk by running

airbyte-ci connectors --use-local-cdk --name=source-faker build
airbyte-ci connectors --use-local-cdk --name=source-faker test

closes #30242
unblocks #30326

alafanechere

Thanks for taking this!
The apply_python_delopment_overrides LGTM but the change in get_cdk_version_from_python_connector look very brittle to me. We already have a function to find_local_dependencies_in_setup_py.
FYI I'm currently refactoring the build/test logic to test connectors from within their real built image: this will allow us to remove the ConnectorPackageInstall step and the environment functions it uses.

alafanechere · 2023-09-15T15:11:27Z

airbyte-ci/connectors/pipelines/pipelines/actions/environments.py

+        connector_container = (
+            connector_container.with_mounted_directory(f"/{path_to_cdk}", directory_to_mount)
+            .with_entrypoint("pip")
+            .with_exec(["install", "--no-deps", "--find-links=.", f"/{path_to_cdk}"])


Instead of overriding the entrypoint you can use the skip_entrypoint flag on with_exec

Suggested change

.with_exec(["install", "--no-deps", "--find-links=.", f"/{path_to_cdk}"])

.with_exec(["pip", "install", "--no-deps", "--find-links=.", f"/{path_to_cdk}"], skip_entrypoint=True)

Ah shoot! I was looking for a flag like that! Thank you!

alafanechere · 2023-09-15T15:12:28Z

airbyte-ci/connectors/pipelines/pipelines/actions/environments.py

+        connector_container = (
+            connector_container.with_mounted_directory(f"/{path_to_cdk}", directory_to_mount)
+            .with_entrypoint("pip")
+            .with_exec(["install", "--no-deps", "--find-links=.", f"/{path_to_cdk}"])


Would you mind adding a comment about the purpose of no-deps and --find-links options?

Comments added!

Also removed --find-links as it was an unessesary relic

alafanechere · 2023-09-15T15:14:23Z

airbyte-ci/connectors/pipelines/pipelines/actions/environments.py

+    if not pip_dependencies:
+        return None
+
+    airbyte_cdk_line = pip_dependencies[0]


Isn't it very brittle to consider the cdk package is always the first in the list?

Ah so originally I was filtering the pip_dependencies down to only those that contained airbyte-cdk

I updated this to make use of next so that we dont need to explicitly get an element at index

alafanechere · 2023-09-15T15:16:28Z

airbyte-ci/connectors/pipelines/pipelines/actions/environments.py

+    pip_dependencies = [dep for dep in pip_freeze_stdout.split("\n") if "airbyte-cdk" in dep]
+    if not pip_dependencies:
+        return None
+
+    airbyte_cdk_line = pip_dependencies[0]
+    if "file://" in airbyte_cdk_line:
+        return "LOCAL"


We already have a function which lists the local dependencies of a connector:
find_local_dependencies_in_setup_py

Ah so this file is based off of what package is installed, and these are retrieved from pip freeze. So we dont parse the file itself

alafanechere

👍 Would appreciate a bit of extra testing but we can do this when refactoring on to of #30474

alafanechere · 2023-09-18T17:47:58Z

airbyte-ci/connectors/pipelines/pipelines/actions/environments.py

@@ -365,6 +365,25 @@ def with_python_connector_source(context: ConnectorContext) -> Container:
    return with_python_package(context, testing_environment, connector_source_path)


+async def apply_python_development_overrides(context: ConnectorContext, connector_container: Container) -> Container:


I'd love to see an unit test on this. I'll implement one when I'll port it on top of #30474

clnoll

Looks good @bnchrch, but I don't think we want to block the CDK from being able to install its own dependencies. Is that a tradeoff that you discussed?

clnoll · 2023-09-18T20:59:45Z

airbyte-ci/connectors/pipelines/pipelines/actions/environments.py

+        context.logger.info(f"Mounting {directory_to_mount}")
+
+        # Install the airbyte-cdk package from the local directory
+        # We use --no-deps to avoid conflicts with the airbyte-cdk version required by the connector


Won't this prevent us from being able to test the local CDK if it has any new/updated dependencies?

For new dependencies, this will throw an exception, but for updates it'll be undefined (i.e. we won't necessarily be testing the code that'll be used in production).

@clnoll great question! and you are exactly right it will ignore sub-dependency conflicts that may crop up.

e.g. if airbyte-cdk needs pandas v2.0.0 but source-s3 need pandas v1.2.3 you wont see the error until you introduce the PR that explicitly bumps source-s3's airbytec-dk version.

The alternative is that we do not include --no-deps and to test local cdk changes a cdk developer would have to

bump the library version number in the pyproject.toml for airbyte-cdk

Ensure/bump the airbyte-cdk library version in the pyproject.toml for a connector you would like to test with to match

Run airbyte-ci --use-local-cdk --name=source-s3 test

The reason I did not choose this route was because

Its an extra step / commit to test different connectors (you have to update their dependency file)

I assumed that the PR that released a new cdk version would not be the same PR that bumped a connector to use the new version. That assumption let me believe the dependency conflicts would be found later in the second PR since you would be running test without --use-local-cdk.

@clnoll more than happy to change the approach, what are your thoughts?

👍 thanks! I want to take a closer look at the code and understand why we’re stuck with this tradeoff between --no-deps and extra steps, given that the original implementation of the local cdk flag didn’t have that tradeoff. This isn’t a super common scenario but it’s common enough that it doesn’t feel quite right to me to skip the deps.

I hear you re the fact that conflicts would be caught before release, but I think that rooting out the issue early is important. Correct me if I'm wrong, but with the current implementation, to fix a dependency conflict I believe we’d have to toggle back and forth between updating the CDK and updating the connector until we settle on a version that agrees with both. Some conflicts could be resolved by doing that locally, but sometimes there are issues that will only crop up in certain development environments. i.e. you’ll see them in CI but not during local development, especially if it involves something like numpy or pandas that has a C dependency - and conflicts related to those libraries are the biggest sources of these problems that I've seen.

I want to take a closer look at the code and understand why we’re stuck with this tradeoff between --no-deps and extra steps

Ah whoops forgot to put a link to part of the why:
jazzband/pip-tools#215

the original implementation of the local cdk flag didn’t have that tradeoff

I can answer that!

So the original implementation used sed to edit the setup.py file using find and replace.

This approach is both brittle and a victim of the python ecosystem where there are approximately 5 "standard" ways in 2023 to define your dependencies

Using requirements.txt

Using setup.py

using setup.py + requirements.txt

Using pyproject.toml

Using pyproject.toml + poetry.lock

Were currently planning on forcing python connectors to all move to the "pyproject.toml + poetry.lock" method but thats still loose plans and today were stuck with 5 possible ways that the airbyte-cdk dependency is declared.

To continue to use the sed approach we would have to have parsing and local override code for all 4 of these files.

However, because they all end up using pip at the end of the day we chose to

Let the connector install using any of the above methods (for now)

Call pip install path/to/airbyte-cdk/python after the fact.

As the trade off is to either

Ignore connector vs airbyte-cdk dependencies conflict during cdk testing

Or, require you to update a connectors dependency file when testing the cdk

Correct me if I'm wrong, but with the current implementation, to fix a dependency conflict I believe we’d have to toggle back and forth between updating the CDK and updating the connector until we settle on a version that agrees with both

🤔 I dont think you'd have to toggle back and forth.

Let me explain

Lets assume when your updating a connector to use a new version of the cdk you have this goal in mind

Upgrade to the latest possible version of cdk that wont require a massive refactor

WIth this you'll likely target latest cdk.

If theres a dependency conflict:

you will look at upgrading your own depenencies first

If you cant upgrade your dependencies (You need an old version of pandas or something):

you will likely walk back a version of the airbyte-cdk and try again.

I dont think a connector dev would be messing with cdk dependency versions
and vice versa, I dont think a cdk dev would be messing with a connector version (if theres a major conflict, try testing with another connector)

@clnoll Is that a fair way to think about it?

but sometimes there are issues that will only crop up in certain development environments. i.e. you’ll see them in CI but not during local development, especially if it involves something like numpy or pandas that has a C dependency - and conflicts related to those libraries are the biggest sources of these problems that I've seen.

Were hoping this method solves this very issue where airbyte-ci connectors build, airbyte-ci connectors test, and airbyte-ci connectors publish both use the same, repeatable (dockerized) environment both on your machine and in CI

Thanks so much for the details, @bnchrch!

I agree with your description of how a connector dev would be thinking about this situation. Re the CDK dev perspective - we do test changes on existing connectors and I could see us wanting to make sure that a package upgrade was appropriate for specific connectors, particularly if it's a popular connector, or if a CDK update was made with solving problems specific to those connectors.

But, if we're confident that airbyte-ci is reproducing the prod environment I think we're good to just deal with this scenario locally.

bnchrch requested a review from a team September 15, 2023 02:07

bnchrch requested a review from a team as a code owner September 15, 2023 02:07

octavia-squidington-iii added the CDK Connector Development Kit label Sep 15, 2023

bnchrch mentioned this pull request Sep 15, 2023

Airbyte-ci: Remove gradle task connectorAcceptanceTest #30326

Merged

alafanechere requested changes Sep 15, 2023

View reviewed changes

bnchrch force-pushed the bnchrch/ci/local-cdk branch from ce08165 to 8f66e1c Compare September 15, 2023 18:51

bnchrch added 8 commits September 15, 2023 12:02

rough pass on using the local cdk

9a86bf8

Clean up the code slightly

3402a84

Ensure that tests work as expected

2488fea

Update changelog

42621ce

Format

1353db1

Update readme for python cdk

f9ef67c

Add pr link

5a22a2a

Apply code review changes

390f6b1

bnchrch force-pushed the bnchrch/ci/local-cdk branch from 8f66e1c to 390f6b1 Compare September 15, 2023 19:04

bnchrch requested a review from alafanechere September 15, 2023 19:06

alafanechere approved these changes Sep 18, 2023

View reviewed changes

clnoll requested changes Sep 18, 2023

View reviewed changes

clnoll approved these changes Sep 19, 2023

View reviewed changes

bnchrch merged commit 07a7989 into master Sep 19, 2023
26 checks passed

bnchrch deleted the bnchrch/ci/local-cdk branch September 19, 2023 19:27

alafanechere mentioned this pull request Sep 20, 2023

A single base for all python connectors #30231

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Airbyte-ci: Add local cdk support #30461

Airbyte-ci: Add local cdk support #30461

bnchrch commented Sep 15, 2023

alafanechere left a comment

alafanechere Sep 15, 2023

bnchrch Sep 15, 2023

alafanechere Sep 15, 2023

bnchrch Sep 15, 2023

alafanechere Sep 15, 2023

bnchrch Sep 15, 2023

alafanechere Sep 15, 2023

bnchrch Sep 15, 2023

alafanechere left a comment

alafanechere Sep 18, 2023

clnoll left a comment

clnoll Sep 18, 2023

bnchrch Sep 18, 2023

clnoll Sep 18, 2023

bnchrch Sep 18, 2023 •

edited

Loading

clnoll Sep 19, 2023

	.with_exec(["install", "--no-deps", "--find-links=.", f"/{path_to_cdk}"])
	.with_exec(["pip", "install", "--no-deps", "--find-links=.", f"/{path_to_cdk}"], skip_entrypoint=True)

		@@ -365,6 +365,25 @@ def with_python_connector_source(context: ConnectorContext) -> Container:
		return with_python_package(context, testing_environment, connector_source_path)


		async def apply_python_development_overrides(context: ConnectorContext, connector_container: Container) -> Container:

Airbyte-ci: Add local cdk support #30461

Airbyte-ci: Add local cdk support #30461

Conversation

bnchrch commented Sep 15, 2023

Overview

alafanechere 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

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnoll 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

bnchrch Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnchrch Sep 18, 2023 •

edited

Loading