Repository navigation
fix(sdm): precompile dependency bytecode in the SDM image - #1222
Conversation
…nifest image Co-Authored-By: bot_apk <apk@cognition.ai>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1791497733-sdm-precompile-bytecode#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1791497733-sdm-precompile-bytecodePR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
|
/prerelease
|
|
Checked the prerelease image published by the This means the publish pipeline keeps source mtimes and the timestamp-based I also timed
|
Patrick Nilan (pnilan)
left a comment
Patrick Nilan (pnilan)
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI Agent.
Three changes to make before this merges. The bytecode itself checks out: all 6,596 precompiled code objects match what compiling from source produces, and spec, check, discover and read output is unchanged.
1. Don't fail the SDM build when a file doesn't compile (Dockerfile:26)
compileall exits 1 when any file fails to compile, so this line fails the SDM image build on any uncompilable .py in site-packages. Nothing fails today, but a dependency update could add such a file. Then:
SDM Docker Image Buildisn't a required check onmain, so the update can still merge.- At the next release,
publish_cdkandpublish_sdmeach need onlybuild. PyPI would get the new version, the SDM image build would fail, and Builder sources and manifest-only connectors would stay on the previous SDM version. Nothing alerts on a failedpublish_sdmtoday; that is tracked separately in airbytehq/airbyte-internal-issues#18062.
Make the release build warn instead of fail, and drop "Fails the build if any file fails to compile." from the comment above it:
RUN python -m compileall -q -j 0 /usr/local/lib/python3.13/site-packages \
|| echo "WARNING: some files failed to compile (see errors above); continuing without their bytecode"The CI check still reports any such file as missing a .pyc, so a PR that adds one still goes red.
2. Reject .pyc files the check can't validate (.github/workflows/docker-build-check.yml:76-78)
The check compares mtime and size only when flags == 0, and passes any other .pyc without comparing. Reproduced against a test image:
- A checked-hash
.pycwith an edited source passes, while Python recompiles that module on every start. - An unchecked-hash
.pycwith an edited source passes, while stale code runs. - A 12-byte header passes, and a 10-byte header crashes the check with
struct.error.
The Dockerfile only produces timestamp-based files, so treat anything else as stale:
if len(header) < 16 or header[:4] != importlib.util.MAGIC_NUMBER or flags != 0:
stale.append(src)
else:
mtime, size = struct.unpack("<II", header[8:16])3. Skip test directories (Dockerfile:26 and the check)
1,372 of the 4,261 new .pyc files are under */test/ or */tests/, mostly in pandas and numpy, and nothing imports them at runtime. Skipping them cuts the added bytecode from 22.7 MB to 13.8 MB compressed, and from 64.9 MB to 37.8 MB on disk, per platform.
Change both places together, or the check fails with 1,372 files missing a .pyc. With fix 1, the Dockerfile step becomes:
# Precompile bytecode for installed packages, skipping test directories. `poetry install` does not
# compile it, and the runtime `airbyte` user cannot write to the root-owned site-packages, so without
# this every connector process recompiles its dependencies on start.
RUN python -m compileall -q -j 0 -x '/tests?/' /usr/local/lib/python3.13/site-packages \
|| echo "WARNING: some files failed to compile (see errors above); continuing without their bytecode"And the check, with fix 2:
import importlib.util
import pathlib
import re
import struct
import sys
import sysconfig
SKIP = re.compile(r"/tests?/") # keep in sync with compileall -x in the Dockerfile
root = pathlib.Path(sysconfig.get_paths()["purelib"])
missing, stale = [], []
total = 0
for src in root.rglob("*.py"):
if SKIP.search(str(src)):
continue
total += 1
pyc = pathlib.Path(importlib.util.cache_from_source(str(src)))
if not pyc.is_file():
missing.append(src)
continue
header = pyc.read_bytes()[:16]
flags = int.from_bytes(header[4:8], "little")
if len(header) < 16 or header[:4] != importlib.util.MAGIC_NUMBER or flags != 0:
stale.append(src)
else:
mtime, size = struct.unpack("<II", header[8:16])
st = src.stat()
if (mtime, size) != (int(st.st_mtime) & 0xFFFFFFFF, st.st_size & 0xFFFFFFFF):
stale.append(src)
print(f"{root}: {total} .py files, {len(missing)} without .pyc, {len(stale)} with stale .pyc")
for path in (missing + stale)[:20]:
print(f" {path}")
sys.exit(1 if total == 0 or missing or stale else 0)Tested
Images built from airbyte/source-declarative-manifest:7.33.0 with the two snippets above:
| Image | Check result |
|---|---|
| With the changes | 5,014 files, 0 missing, 0 stale; exit 0 |
Plus a deliberately broken .py |
Build completes and prints the warning; the check reports 1 missing; exit 1 |
| Compiled as checked-hash | 5,014 stale; exit 1 |
One .pyc truncated to 10 bytes |
1 stale, no crash; exit 1 |
| Published 7.33.0, not precompiled | 2,889 missing; exit 1 |
Importing the SDM entrypoint on the changed image loads 1,477 modules, none from a test directory.
…est dirs Co-Authored-By: bot_apk <apk@cognition.ai>
|
Re: review. I made all three changes in 6574c47, using your snippets as written.
Local results (linux/amd64; the check run is identical to the CI heredoc):
I updated the PR description to match. |
|
Resolve conflicts |
…precompile-bytecode Co-Authored-By: bot_apk <apk@cognition.ai> # Conflicts: # Dockerfile
|
I resolved the conflicts in 7364108, which merges GitHub now shows the PR as mergeable and no new commits have landed on |
Summary
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17922:
Close on merge: yes — internally filed, no customer to confirm with. (The post-release benchmark row for airbytehq/airbyte-internal-issues#17920 is a separate acceptance item.)
Requested by Patrick Nilan (Patrick Nilan (@pnilan)).
The
airbyte/source-declarative-manifestimage ships no.pycfor its dependencies. Poetry 1.8 (poetry install --only main) does not compile bytecode unless--compileis passed; only the CDK wheel (installed withpip) gets.pyc. The container runs asairbyte, which can't write to the root-ownedsite-packages, so every connector process (spec/check/discover/read) recompiles pandas, numpy, pydantic, etc. on start.Changes:
Dockerfile: after the installs and beforeUSER airbyte, addRUN python -m compileall -q -j 0 -x '/tests?/' /usr/local/lib/python3.13/site-packages || echo "WARNING: ...". Usedcompileallrather thanpoetry install --compilebecause it doesn't depend on the installer and covers every package insite-packages. Test directories (*/test/,*/tests/, mostly in pandas/numpy; nothing imports them at runtime) are skipped. A compile failure warns instead of failing the build, so an uncompilable file from a dependency update can't publish the CDK to PyPI while the SDM image build fails; the CI check below still reports any such file as missing a.pyc, so the PR that introduces it goes red..github/workflows/docker-build-check.yml(sdm-docker-build-check): the multi-platform build ispush: falsewith noload, so an extralinux/amd64build withload: true(layers come from the buildx cache) is run, and a check runs inside it that fails if any.pyundersite-packages(outside test directories, same/tests?/pattern as the Dockerfile) has no.pyc, or if a.pycis not a valid timestamp-based file for its source: short header, wrong magic number, hash-based flags, or a timestamp/size header that no longer matches the source (any of which makes Python ignore or bypass it).Timestamps: neither the CDK publish workflow nor the monorepo connector image build sets
rewrite-timestamp/SOURCE_DATE_EPOCH. In the published7.33.0image, the existing pip-compiledairbyte_cdk.pycheaders all still match their sources (the check reports0 with stale .pyc), so the publish pipeline preserves mtimes.Before / after (local, linux/amd64,
--cpus=1, back to back)main)site-packages.py(outside test dirs) without.pycimport source_declarative_manifest.run(median of 5, first revision of this PR)Measured on the first revision of this PR, which compiled everything including test dirs:
source-pokeapibuilt on it, import asairbyte, 2898 ms → 1323 ms;spec3.62 s → 2.11 s; image 326.0 MB. Skipping test dirs did not change import time: in a later run on the same machine, the first revision took a median of 1085 ms and this revision 1054 ms; importing the SDM entrypoint loads 1489 modules, none from a test directory. Thecompilealllayer takes 3.7 s locally.The prerelease image
7.34.0.post2.dev39677594442, published from the first revision, passed the bytecode check on both linux/amd64 and linux/arm64. So the publish pipeline keeps source mtimes and the timestamp-based.pycfiles stay valid.Declarative-First Evaluation
Not applicable: this changes the SDM image build and CI only. No manifest or custom Python component is touched.
Test Coverage
The new CI step is the regression test for this bug. Run locally against images built from
mainand from this branch:main):5019 .py files, 2894 without .pyc, 0 with stale .pyc→ exit 15019 .py files, 0 without .pyc, 0 with stale .pyc→ exit 0.py: the image build completes and prints the WARNING; the check reports 1 missing → exit 1pydanticrecompiled as checked-hash: 105 stale → exit 1.pyctruncated to 10 bytes: 1 stale, no crash → exit 1actionlintpasses on the workflow. No Python unit tests apply; no CDK code changed.Review notes
airbyte_cdk/manifest_server/Dockerfile, the monorepodocker-images/Dockerfile.python-connector, and the existingpoetry install ... || true(Poetry in the built image is broken withModuleNotFoundError: No module named 'filelock'; this PR doesn't change that).Dockerfileauthors): Anatolii Yatsuk (@tolik0), David Gold (@dbgold17).Link to Devin session: https://app.devin.ai/sessions/e58fcb5f001640438eb753278d7bfb4b
Open in Devin Desktop: https://app.devin.ai/desktop/session/e58fcb5f001640438eb753278d7bfb4b?variant=devin