airbyte_ci: up_to_date command

[bleonard]

Meant to be run on a cron script.

Actions:

• Upgrades python connectors to the newest versions of libraries.
• It's meant to (eventually) be run every N days, but can also be helpful in doing mass work.

Examples

• airbyte-ci connectors --name=source-openweather up_to_date: upgrades main dependecies
• airbyte-ci connectors --name=source-openweather up_to_date --dev: forces update if there are only dev changes
• airbyte-ci connectors --name=source-openweather up_to_date --dep pytest@^8.10 --dep airbyte-cdk@0.80.0: allows update to toml files as well

Other things it could do later

• make the pull requests on a well known branch, replacing the last one if still open
• bump the conenctor version and metadata and changelog
• also bump the manifest version of the CDK

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Apr 26, 2024 7:53pm

[clnoll]

Nice @bleonard!

Requesting one change related to the steps_results[-1] indexing, and added a few more minor comments.

[clnoll]

Why do we require it to be a source connector?

[bleonard]

Open to changing later. I felt like I was mostly targeting the source CDK

[clnoll]

Since the poetry.lock file is auto-generated, it feels like for this check we can assert that the connector has a pyproject.toml.

Can we add a check here that the pyproject.toml includes the CDK?

[bleonard]

I'll leave that TODO for my next round. I'd a bit worried about the Candidate notion if this pipeline does lots of things anyway. Not sure it will be helpful if there are N possible things up_to_date could do.

[clnoll]

Poetry has some tools to play around with for this. poetry show -o | awk '{print $1}' | grep airbyte-cdk will work. I didn't immediately see a way to make poetry emit json but might have missed it.

[bleonard]

I did that below. I didn't know if you could do that outside of the container.

[alafanechere]

If you don't want to build the connector container the following shortcuts to suggest

Approach A: compare dependencies.json with the latest cdk verion

1. Check if poetry.lock has changed on the branch by checking the content of self.context.modified_files
2. If poetry.lock was not modified we can consider that the CDK version did not changed compared to the last version.
3. As we archive the connector dependency at build time (for the data team) you can consume the last dependency snapshot of the connector by GETting and parsing this kind of url: https://storage.googleapis.com/prod-airbyte-cloud-connector-metadata-service/connector_dependencies/source-airtable/4.2.0/dependencies.json
4. You can get the latest cdk version by using the get_latest_cdk_version from this module
5. You can parse dependencies.json and compare the installed cdk version to the latest cdk version.

Approach B: pip freeze on the latest released connector

Another approach, which would use a container but without the build footprint could be:

• Keep step 1 and 2 from previous suggestion
• If the lockfile was not updated:

#Instantiate a dagger container with the latest connector image:
latest_connector_container = self.dagger_client.container().from(f"{self.connector.metadata["dockerRepository"]}:latest")
# The following line will pull the connector image if it's not cached. It has some network overhead
installed_dependencies = (await latest_connector_container.with_exec(["pip", "freeze"], skip_entrypoint=True).stdout()).splitlines()
cdk_version = [dependency.split("==")[1] for dependency in installed_dependencies if "airbyte-cdk" in dependency][0]
latest_cdk_version = get_latest_cdk_version()

Approach C [preferred?] : parse the poetry.lock file

lockfile_content = await self.context.get_connector_dir().file("poetry.lock").contents()

Then you can use regex or this library to parse and query the lockfile and finally compare the cdk version with the output of get_latest_cdk_version()

[clnoll]

This feels like it belongs in CheckIsUpdateCdkCandidate. Any reason not to put it there?

[bleonard]

This was back to when I spun up the container. I tried to do it all in one spot. Is that common?

[clnoll]

Got it, that makes sense to me.

[alafanechere]

I feel like these checks do belong here and not in a separate step. I would also suggest placing here the "check if the current cdk version is the latest and skip if it is" logic.
It will lead to a more self contained and explanatory Step: Why are we skipping the UpdateCDK execution? because the connector is already on the latest version sounds simpler to understand compared to Why did UpdateCDK did not run? > Because CheckIsUpdateCdkCandidate was skipped.

[bleonard]

In the world we are moving to that will do several (but not all) of 7 things, should there be 7 different checks like this? Or should there be 7 tasks that might return SKIPPED - i lean towards the later because otherwise, you end up with duplicated logic between the Candidate step and the ActuallyDoIt step.

[clnoll]

I haven't used it myself but it looks like packaging.version.Version does this.

[alafanechere]

I'd suggest using the semver package, you can reuse some logic from the VersionCheck step

[bleonard]

random note I learned semver crashes with valid PyPi versions like "2.4" and "2.3.1.dev1" - I'm going to use use the python packaging.version as it's more likely to work as expected.

[alafanechere]

We already have a command to ugprade the cdk for python and java connectors (logic here doc here). It has not been updated to support poetry though.

I expected the up_to_date command to not only update the cdk but also all other dependencies of the connector if they exist.
It would "simply" be a "à la Dependabot" global dependecy update with: poetry update.
poetry update would upgrade all the dependencies while still respecting the dependency upper bounds defined in pyproject.toml.

I'm requesting changes because I'd appreciate either:
A. A rework of the upgrade_cdk command to support poetry.
B. A new command which would update all the dependencies, not only the CDK.

[alafanechere]

Suggested change

	"""Check if the connector is a candidate for migration to poetry.
	"""Check if the connector is a candidate for CDK update.

[bleonard]

The plan is to work this in to the whole dependabot thing. The stuff I mentioned in the PR. Is it not possible to check in current progress?

[alafanechere]

@bleonard we could check in current progress but what I don't get is why we should consider the CDK a specific dependency, different from the ones that would be updated with a single poetry update execution.

From your PR description if think it don't get the else of do a poetry update to update everything else.

I'd be happier merging a new command which just does poetry update on a connector and exports back the updated poetry.lock file. It would introduce a new command which has a net new feature compared to the existing upgrade-cdk one.

[bleonard]

Well I feel like we should set the CDK explicitly so doing a poetry update wouldn't fix it. It's easy to set that explicitly and clarity on CDK conpatibility is important. * or ^0 won't provide that. I can add the Poetry update command now if that's the request.

[alafanechere]

we should set the CDK explicitly

Ahh makes sense, this is what I did not get. Let me double check with @girarda that this is what we want.

I can add the Poetry update command now if that's the request.

This can be a follow up PR if the update to a specific CDK version is what we want instead of "always latest".

[alafanechere]

nit: if the pipeline is the language agnostic but the Step is not I would suggest creating language specific submodules under a step subpackage like we did for build. You could argue that it's not something I respected for migrate_to_poetry and migrate_to_base_image commands.

[bleonard]

Ok, I copied migrate_to_poetry basically.

[alafanechere]

nit: we should have an helper to check whether the connector is poetry based

[alafanechere]

There's a significant code duplication with CheckIsMigrationCandidate on the poetry pipeline in this step. Can we create a parent class for both steps?
I also realize that these step are not concretely performing anything. Capturing this logic inside a function which would early exit the pipeline might be more straightforward.

[alafanechere]

If you don't want to build the connector container the following shortcuts to suggest

Approach A: compare dependencies.json with the latest cdk verion

1. Check if poetry.lock has changed on the branch by checking the content of self.context.modified_files
2. If poetry.lock was not modified we can consider that the CDK version did not changed compared to the last version.
3. As we archive the connector dependency at build time (for the data team) you can consume the last dependency snapshot of the connector by GETting and parsing this kind of url: https://storage.googleapis.com/prod-airbyte-cloud-connector-metadata-service/connector_dependencies/source-airtable/4.2.0/dependencies.json
4. You can get the latest cdk version by using the get_latest_cdk_version from this module
5. You can parse dependencies.json and compare the installed cdk version to the latest cdk version.

Approach B: pip freeze on the latest released connector

Another approach, which would use a container but without the build footprint could be:

• Keep step 1 and 2 from previous suggestion
• If the lockfile was not updated:

#Instantiate a dagger container with the latest connector image:
latest_connector_container = self.dagger_client.container().from(f"{self.connector.metadata["dockerRepository"]}:latest")
# The following line will pull the connector image if it's not cached. It has some network overhead
installed_dependencies = (await latest_connector_container.with_exec(["pip", "freeze"], skip_entrypoint=True).stdout()).splitlines()
cdk_version = [dependency.split("==")[1] for dependency in installed_dependencies if "airbyte-cdk" in dependency][0]
latest_cdk_version = get_latest_cdk_version()

Approach C [preferred?] : parse the poetry.lock file

lockfile_content = await self.context.get_connector_dir().file("poetry.lock").contents()

Then you can use regex or this library to parse and query the lockfile and finally compare the cdk version with the output of get_latest_cdk_version()

[alafanechere]

Are we mounting these files because they are not kept after build on the built connector?

[bleonard]

I don't know anything about mounting. I was just copying what you did in the migrate_to_poetry action. Is this what it means: You should mount things that might have changed since the container was made ?

[alafanechere]

I'd suggest using the semver package, you can reuse some logic from the VersionCheck step

[bleonard]

I've updated this with sevral changes based on the Slack discussions. It will now expclity move the airbyte-cdk pin so that PyAirbyte and others using pip will work predictably. It also has a place to handle the logic for which version that can be updated to. It will poetry update the rest.

This is not getting called anywhere (low risk) and it being on this branch is blocking me from working on all the connector PRs in another branch. If there is nothing scary, can we merge and keep a list for later updates?

[alafanechere]

Look like a good enough first iteration on this command 👍 Thanks for your patience with this code base. Can you please bump the tool version in pyproject.toml and add a changelog entry in the README? 🙏

There are room for logic consolidation but I suggest to tackle it once we want to automatically orchestrate it and make it do other things.

I left a couple of other suggestions. But as you said, nothing in this branch is in the critical path so feel free to merge if you need to manage your lots of connectors PRs.

[alafanechere]

TIL

[alafanechere]

nit: I find the name of this class not matchin what it does. UpdateConnectorDependencies?

[alafanechere]

Suggested change

	- The original dependencies are installed in the new connector image.
	- The dev dependencies are not installed in the new connector image.

[alafanechere]

If I followed the code path correctly we'd get a dict with the following structure:

{
  "airbyte-cdk: "airbyte-cdk==0.80.0"
}

Am I correct? (would be worth adding a docstring)

[bleonard]

That's right. the second part being what would be poetry add

[alafanechere]

Could this section execution be conditioned by a flag passed to UpdatePoetry to optionally run a full poetry update (this could also be a different step in the pipeline)?

[bleonard]

You think there's a reason to be optional? I'm pretty open to that.

[alafanechere]

Can we remove this else block and instantiate a second UpdatePoetry step in the run_up_to_date_pipeline, the pipeline would pick the right cdk version (according to inputs or by fetching the latest version).
I would remove "business" logic from the Step and make it more reusable - self contained.

In other words: can we make self.specified_versions not optional 😄

[bleonard]

Maybe. But I don't thin kthe eventual cron script will specify the versions for all the updates that should be poetry update --- specified_versions was really only for local work. "Let's change try changing the semantic version of pytest" kind of stuff.

[natikgadzhi]

You mean only dev dependencies are changed?