build(airbyte-ci): Test python versions matrix #40731

inossidabile · 2024-07-04T09:07:17Z

What

airbyte-ci test command was made to pick up the python version from the package pyproject.toml. Allowed running several versions at once.

How

[tool.airbyte_ci]
python_versions = ["3.11.5", "3.10.12"]

Review guide

🤷

User Impact

None

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2024-07-04T09:07:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jul 9, 2024 9:12am

inossidabile · 2024-07-04T09:10:56Z

@natikgadzhi 🕺

natikgadzhi

Thank you! <3

I like this. Our CI checks if you changed airbyte-ci itself and refuses to run tests on PRs from forks if you did, because security.

I'll have to copy your PR over and spin up the CI, and test locally.

@alafanechere, let's pair on this tomorrow for a bit and see if we can get this in easily. Perhaps running tests on CDK on 3.11 could be a low hanging fruit, and then it would make sense to upgrade everything!

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

natikgadzhi · 2024-07-05T05:03:11Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/models.py


 from pydantic import BaseModel, Field, validator


+DEFAULT_PYTHON_VERSION = "3.10.12"


Ironically, while the default here is / was 3.10, connectors run 3.9 today. Nothing for you to change, just our mistake.

They still do. Connector testing is a bit on the side compared to so-called component testing. That's different commands on airbyte-ci and they bootstrap dagger differently.. From what I figured, it's probably cause you have prebuilt Docker to inherit for components that they run into. I wonder if it's only used for testing or if it's a part of distribution and deployment. If it's test-only, we can probably pack it on the fly. If it's not, we need to make tagged versions of it to supply all demand anyway. So it's looking good.

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

alafanechere

Thanks for the initiative!
It mostly LGTM 😄
I'm requesting change as I'd like to minimize the awareness of airbyte-ci about python versions. I'd like the AirbyteCiPackageConfiguration to be the single interface.
I'll 👍 if you agree to:

Remove the default python version
Enforce a python version to be declared in pyproject.toml (and update internal packages to set it)
Remove the new option which allows overriding pyproject.toml

alafanechere · 2024-07-05T05:55:50Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

@@ -113,7 +113,7 @@ def get_poetry_base_container(dagger_client: dagger.Client) -> dagger.Container:
    poetry_cache_path = "/root/.cache/poetry"
    return (
        dagger_client.container()
-        .from_("python:3.10.12")
+        .from_(f"python:{python_version}")


I'd rather create an explicit mapping between python versions and images in case we want to use other images/tags.

Love the idea, will pick up

I was thinking a lot about this one and this is what I ended up with: https://github.com/inossidabile/airbyte/pull/1/files#diff-b86e6d2f27d70812919004f8b786a5bb0ea5a7a28ec99c5ffd8773d9eb424eecR44. Can't get over internal "overengineered" alarm. Also, what do you think about using it to map minor versions to patch?

I decided to go ahead and push the change. Let's go through it in this PR directly :).

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/models.py

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/commands.py

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/pipeline.py

inossidabile · 2024-07-05T06:42:36Z

@alafanechere I really like the idea of inferring the matrix from classic dependency syntax btw. That makes perfect sense. We pick up the lowest major version and take the latest release of every major release in the allowed range. Duplication is never cool :)

inossidabile · 2024-07-08T20:42:30Z

The more I understand how airbyte-ci is invoked in Github Actions, the more I understand your point about not having --python-version. I dropped it because using it on CI isn't a short reach.

However, I still think that forking jobs into a matrix is a good idea but it has to be done this way:

Workflow should put in the specific parameters it wants to run with (e.g. --modified) to a special new argument. Let's call it --plan which would return a list of arguments for every package and its python versions it'd run. For instance: airbyte-ci test --modified --plan

["-p airbyte-cdk/python --python-version=3.10", "-p airbyte-cdk/python --python-version=3.11", "airbyte-ci test -p airbyte-ci/connectors/pipelines --python-version=3.10"]

Then we use the output as a matrix on workflow like this: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#example-using-an-output-to-define-two-matrices

If I understood you well, that won't create redundancy in the Github Actions definition and will still be controlled entirely from pyproject.toml just like we agreed it should be. And the main benefit of doing that is distinct test runs for each version and package which are way more manageable, transparent, and give you a lot of flexibility in protection rules, etc. Let's look into it a separate task/PR if you don't mind.

alafanechere

Thank you @inossidabile for all these changes!
It LGTM but I need to test it locally before 👍 : CI on this branch is using the airbyte-ci latest binary, it's not built from your branch source (it's for security on our community CI) .

About this:

I still think that forking jobs into a matrix is a good idea but it has to be done this way:

I don't think it's a bad idea but I continue to believe that more granular status check can be emitted from airbyte-ci directly and not from GHA. In the same manner we currently emit per package status check we could increase the granularity per python version. I prefer keeping things in airbyte-ci as we try to centralize the control flow as much as possible.

alafanechere · 2024-07-08T13:54:01Z

airbyte-cdk/python/pyproject.toml

@@ -98,6 +98,7 @@ check-ci = {sequence = ["build", "lint", "unit-test-with-cov"], help = "Build th
 pre-push = {sequence = ["build", "check-local"], help = "Run all build and check tasks."}

 [tool.airbyte_ci]
+python_versions = ["3.11.5", "3.10.12"]


Suggested change

python_versions = ["3.11.5", "3.10.12"]

python_versions = ["3.11.5", "3.10.12", "3.9"]

alafanechere · 2024-07-09T08:48:08Z

airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/test/models.py

@@ -20,6 +20,7 @@ class AirbyteCiPackageConfiguration(BaseModel):
        False,
        description="Flag indicating the mount of the host docker socket to the container running the poe task, useful when the package under test is using dagger",
    )
+    python_versions: List[str] = Field([], description="List of unique python versions to run the poe tasks on")


Suggested change

python_versions: List[str] = Field([], description="List of unique python versions to run the poe tasks on")

python_versions: List[str] = Field(description="List of unique python versions to run the poe tasks on")

This will make the python_versions field required and raise a ValidationError. I think its better to have validation at this level instead of checking if python_versions is set in the pipeline.py and raise a click.UsageError

Oops :). Sorry, I didn't familiarize myself with this part enough. Thanks for the advice.

…odel

alafanechere

I tested this locally and it works as expected 👍

inossidabile requested a review from a team as a code owner July 4, 2024 09:07

octavia-squidington-iii added CDK Connector Development Kit community labels Jul 4, 2024

inossidabile force-pushed the test-python-versions-matrix branch 2 times, most recently from 32c1a2a to b562774 Compare July 4, 2024 09:09

inossidabile changed the title ~~Test python versions matrix~~ build(airbyte-ci): Test python versions matrix Jul 4, 2024

inossidabile force-pushed the test-python-versions-matrix branch from b562774 to 5db8de0 Compare July 4, 2024 09:22

Ability to specify python version(s) to run for specific package added

9a0b66a

inossidabile force-pushed the test-python-versions-matrix branch from 5db8de0 to 9a0b66a Compare July 4, 2024 09:26

marcosmarxm added the team/tooling label Jul 4, 2024

natikgadzhi reviewed Jul 5, 2024

View reviewed changes

alafanechere requested changes Jul 5, 2024

View reviewed changes

natikgadzhi mentioned this pull request Jul 7, 2024

fix(destination-qdrant): Update to CDK 2.0 and qdtant-client 1.10 #41020

Merged

2 tasks

inossidabile added 2 commits July 7, 2024 18:50

More restricted and pyproject.toml focused version management

f30625a

More restricted and pyproject.toml focused version management

8d9b73c

alafanechere mentioned this pull request Jul 9, 2024

community-ci: remove airbyte-ci from protected paths #41058

Merged

Merge branch 'master' into test-python-versions-matrix

ef6008c

alafanechere temporarily deployed to community-ci-auto July 9, 2024 08:32 — with GitHub Actions Inactive

alafanechere temporarily deployed to community-ci July 9, 2024 08:32 — with GitHub Actions Inactive

alafanechere temporarily deployed to community-ci-auto July 9, 2024 08:32 — with GitHub Actions Inactive

alafanechere had a problem deploying to community-ci July 9, 2024 08:32 — with GitHub Actions Failure

vercel bot deployed to Preview July 9, 2024 08:36 View deployment

alafanechere reviewed Jul 9, 2024

View reviewed changes

make python_version field required in AirbyteCiPackageConfiguration m…

53c9aa7

…odel

alafanechere temporarily deployed to community-ci-auto July 9, 2024 09:12 — with GitHub Actions Inactive

alafanechere requested a deployment to community-ci July 9, 2024 09:12 — with GitHub Actions Waiting

alafanechere temporarily deployed to community-ci-auto July 9, 2024 09:12 — with GitHub Actions Inactive

alafanechere requested a deployment to community-ci July 9, 2024 09:12 — with GitHub Actions Waiting

alafanechere approved these changes Jul 9, 2024

View reviewed changes

alafanechere merged commit 1c4ff2a into airbytehq:master Jul 9, 2024
25 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(airbyte-ci): Test python versions matrix #40731

build(airbyte-ci): Test python versions matrix #40731

inossidabile commented Jul 4, 2024 •

edited by natikgadzhi

Loading

vercel bot commented Jul 4, 2024 •

edited

Loading

inossidabile commented Jul 4, 2024 •

edited

Loading

natikgadzhi left a comment

natikgadzhi Jul 5, 2024

inossidabile Jul 5, 2024

alafanechere left a comment

alafanechere Jul 5, 2024

inossidabile Jul 5, 2024

inossidabile Jul 8, 2024

inossidabile Jul 8, 2024

inossidabile commented Jul 5, 2024

inossidabile commented Jul 8, 2024 •

edited

Loading

alafanechere left a comment

alafanechere Jul 8, 2024

alafanechere Jul 9, 2024 •

edited

Loading

inossidabile Jul 9, 2024

alafanechere left a comment


		from pydantic import BaseModel, Field, validator


		DEFAULT_PYTHON_VERSION = "3.10.12"

	python_versions = ["3.11.5", "3.10.12"]
	python_versions = ["3.11.5", "3.10.12", "3.9"]

	python_versions: List[str] = Field([], description="List of unique python versions to run the poe tasks on")
	python_versions: List[str] = Field(description="List of unique python versions to run the poe tasks on")

build(airbyte-ci): Test python versions matrix #40731

build(airbyte-ci): Test python versions matrix #40731

Conversation

inossidabile commented Jul 4, 2024 • edited by natikgadzhi Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Jul 4, 2024 • edited Loading

inossidabile commented Jul 4, 2024 • edited Loading

natikgadzhi left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inossidabile commented Jul 5, 2024

inossidabile commented Jul 8, 2024 • edited Loading

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere Jul 9, 2024 • edited Loading

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

inossidabile commented Jul 4, 2024 •

edited by natikgadzhi

Loading

vercel bot commented Jul 4, 2024 •

edited

Loading

inossidabile commented Jul 4, 2024 •

edited

Loading

inossidabile commented Jul 8, 2024 •

edited

Loading

alafanechere Jul 9, 2024 •

edited

Loading