Repository navigation
fix: defer pandas import in ResponseToFileExtractor - #1221
Conversation
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/1791494270-defer-pandas-import#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/1791494270-defer-pandas-importPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Patrick Nilan (pnilan)
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI Agent.
Review: fix before merge
The change is sound and non-breaking. Moving the pandas import into _read_with_chunks stops every declarative and SDM process from loading pandas and numpy at start-up, and CSV output is unchanged. Two one-line fixes are required before merge. The rest are recommendations.
| # | Priority | Where | Ask |
|---|---|---|---|
| 1 | Required | PR title | Change perf: to an allowed type |
| 2 | Required | unit_tests/sources/declarative/extractors/test_response_to_file_extractor.py |
Import pandas at module level, so the memory test measures parsing rather than the lazy import |
| 3 | Recommended | airbyte_cdk/sources/declarative/extractors/response_to_file_extractor.py:142 |
Delete the temp file if the pandas import fails |
| 4 | Recommended | Dockerfile:21 |
Smoke-import pandas in the SDM image |
| 5 | Recommended | unit_tests/source_declarative_manifest/test_source_declarative_manifest_imports.py:47 |
Pin the subprocess's working directory |
| 6 | Nit | unit_tests/source_declarative_manifest/test_source_declarative_manifest_imports.py:43 |
Docstring, failure output, reuse fixtures |
Required
1. Retitle the PR to an allowed type
perf is not in the allowed list in .github/workflows/semantic_pr_check.yml:27-41, which accepts fix, feat, docs, ci, chore, build and tests.
- The required
Validate PR titlecheck is green only because both of its steps skip drafts (:20,:44). - The check re-runs on
ready_for_review(:9) and will fail then.
Suggested title, following the precedent of #900 (fix(cli): make test imports lazy …):
fix: defer pandas import in ResponseToFileExtractor
perf, fix and chore all release as a patch, so the version is unaffected.
2. Keep the 20 MB memory test measuring parsing
test_response_to_file_extractor_memory_usage (test_response_to_file_extractor.py:115-128) carries @pytest.mark.limit_memory("20 MB"). pytest-memray tracks any test with that marker, even without --memray.
- Before this PR: pandas loaded when the module was imported, outside the measured call.
- Now: if nothing earlier in the process has loaded pandas, the import happens inside the measured call at line 125.
CI passes only because pytest collection and the six earlier tests in the file load pandas first. When the test runs on its own, it fails. Measured on Linux with Python 3.11, pandas 2.3.3, memray 1.20.0 and pytest-memray 1.7.0:
| Invocation | main |
This PR | This PR + fix |
|---|---|---|---|
pytest …::test_response_to_file_extractor_memory_usage |
pass | fail: 43.2–44.2 MiB allocated, limit 20 MiB | pass |
pytest -m slow …/test_response_to_file_extractor.py |
not run | fail: 45.2 MiB | pass |
| Whole file | 7 passed | 7 passed | not run |
Fix (verified in the table above): add the first line to the file's imports.
import pandas # noqa: F401 # loaded here so limit_memory measures CSV parsing, not the extractor's lazy import
import pytest
import requests
import requests_mockRecommended
3. Delete the temp file if the pandas import fails
_read_with_chunks is a generator, so line 142 runs only after extract_records has called _save_to_file (line 189). By then the whole response is already on disk as ./<uuid4>. The import sits outside the try: at line 144, so if it raises, the finally: os.remove(path) at lines 167-169 never runs.
What happens with pandas made unimportable:
- One async job at a time: every partition left its downloaded file behind.
- Concurrent async jobs: some partitions reported
AttributeError: module 'pandas' has no attribute 'errors'instead ofImportError.
This only affects a broken install, so the fix is optional. Please keep the import out of the existing try:. Inside it, a failed import would surface as UnboundLocalError from except pd.errors.EmptyDataError. A minimal guard to replace line 142:
try:
import pandas as pd
except ImportError:
os.remove(path)
raiseIf you'd rather not add it, mention the leftover file in the PR body's trade-off note.
4. Smoke-import pandas in the SDM image
- Before this PR: every SDM command imported pandas. The connector image tests that run on the branch-built SDM image (
connector-tests.yml:202-235) therefore also proved that pandas imports inside the image. - After this PR: nothing in CI imports pandas inside the image. In the CI pytest runs, pandas is already loaded during collection, so line 142 never performs a first import.
- Why it matters:
Dockerfile:18ends in|| true, so the image can drift from the lockfile without anyone noticing.
One line after Dockerfile:21 restores the check:
RUN python -c "import pandas"Optionally, also extend the new test's SCRIPT after discover:
- Mock a small CSV download.
- Run
ResponseToFileExtractoron it. - Assert the records, and that
"pandas" in sys.modules.
5. Pin the subprocess's working directory
subprocess.run([sys.executable, "-c", SCRIPT, ...]) at line 47 sets neither cwd nor env. With -c, Python looks for airbyte_cdk in the current directory first, then in the installed package.
- From another directory: the subprocess can import a different
airbyte_cdkthan the one pytest collected. I reproduced a false failure this way. - In a worktree that shares a venv: the same mismatch can produce a false pass.
- CI: unaffected, because it runs from the repo root.
result = subprocess.run(
[sys.executable, "-c", SCRIPT, str(manifest_path), str(config_path)],
cwd=Path(__file__).resolve().parents[2],
capture_output=True,
text=True,
timeout=120,
)Nits
6. Make the new test explain itself
- Docstring. Add one stating the rule the test enforces: no module-level pandas or numpy import reachable from the SDM entrypoint. Also state the remedy: import inside the function that needs it.
python -X importtime -c "import airbyte_cdk.cli.source_declarative_manifest._run"shows which module pulls the library in. - Failure output.
next(...)at lines 57-59 has no default, so a missing marker line raises a bareStopIteration. Usenext(..., None)and assert withresult.stdoutandresult.stderrin the message.- Do the same for the asserts at lines 55-56, because the subprocess exits 0 even when
checkfails.
- Fixtures. Take the conftest fixtures
valid_local_manifestandvalid_local_config_fileas arguments, instead of rebuilding the paths at lines 44-45.
PR body
- Acceptance criterion 3 (source-salesforce bulk streams still parse CSV) has no evidence in the PR yet. In this review, source-salesforce's unit tests passed 159/159 against this branch's CDK, including
unit_tests/integration/test_bulk_stream.py23/23. Please record this or equivalent evidence. - Trade-off note, if #3 isn't applied: with a broken pandas install,
checknow passes, and only CSV streams fail at read time, leaving the downloaded file on disk.
Verified, no action needed
- Non-breaking. There is no schema, factory, state, default or dependency change. The removed module-level
pdalias isn't referenced in this repo, in the connector monorepo, or anywhere an org-wide code search finds. - CSV output. Byte-identical to
mainacross 60 cases, coveringpreserve_na_valueson and off, gzip, null bytes, NA tokens and an empty body. - Concurrency. First imports of pandas on worker threads were safe across 2,000 fresh-process runs on Python 3.10–3.14.
- The new test. It fails on
mainwith['numpy', 'pandas']and passes here. The remote-manifest SDM path also loads neither module. - CI. All green except
Check: destination-motherduck, which fails while fetching its connector secret. #1242 and #1239 fail the same way, so it is unrelated to this PR. - Side note, out of scope here.
.claude/skills/create-pr/SKILL.md:43-54listsperf,refactor,style,testandrevertas allowed types, but the title check rejects all five. That is probably where theperf:title came from.
Co-Authored-By: bot_apk <apk@cognition.ai>
|
Thanks Patrick Nilan (@pnilan). I've addressed all six items in 5d732de.
The PR body now includes your source-salesforce run (159/159, with |
Summary
response_to_file_extractor.pyimported pandas at module level. That module is always loaded on the declarative / SDM import path (extractors/__init__.py,model_to_component_factory.py,http_job_repository.py). As a result, every manifest-only connector process imported pandas and numpy at start-up, on every command, even though none of them use either library. pandas is only used inResponseToFileExtractor._read_with_chunks.The CDK already uses the same lazy-import approach for
unstructuredinfile_based/file_types/unstructured_parser.py. Behavior is unchanged, includingpreserve_na_values. The only consumer in the monorepo issource-salesforce, a Python connector. Its tests patchsource_salesforce.streams.ResponseToFileExtractor, not the module'spd.Measured locally:
main, importingairbyte_cdk.cli.source_declarative_manifest._runleaves bothpandasandnumpyinsys.modules(True True).-X importtimeattributes about 198 ms cumulative toresponse_to_file_extractor.False False, and the cumulative import time is about 0.4 ms.Other changes:
Dockerfile: addsRUN python -c "import pandas"after the wheel install. SDM commands no longer import pandas, so this is what still checks that pandas is importable in the image.test_response_to_file_extractor.py: imports pandas at module level. Otherwise thelimit_memory("20 MB")test would count the lazy import, about 44 MiB, when run alone.Trade-off: with a broken pandas install,
checknow passes, and only CSV-extractor streams fail at read time, withImportError. The Dockerfile smoke import and the extractor unit tests cover this case.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17921:
Close on merge: yes — internally filed, no customer to confirm with.
Part of https://github.com/airbytehq/airbyte-internal-issues/issues/17920 (faster
checkfor manifest-only connectors).Test Coverage
unit_tests/source_declarative_manifest/test_source_declarative_manifest_imports.py:spec,check(HTTP mocked withrequests_mock) anddiscoveron the PokeAPI test manifest in a subprocess, withcwdpinned to the repo root.pandasnornumpywas imported.["numpy", "pandas"]) and passes with it.pd.read_csv, includingpreserve_na_values. The memory test passes when run alone.unit_tests/source_declarative_manifest/, the extractor tests andunit_tests/sources/declarative/requesters/pass.ruffandmypyare clean.ImportErrorpropagates and the temp file is deleted.unit_tests/integration/test_bulk_stream.py(23/23). I didn't re-run these myself.Declarative-First Evaluation
N/A. This is a CDK-internal import change, not a change to a connector manifest or custom component.
Breaking Change Evaluation
Not breaking: no schema, spec, state, stream or data-scope change. CDK versions are set by the release tooling, so this PR has no version bump.
Link to Devin session: https://app.devin.ai/sessions/919705a00f754ff494dff1d2ed6f9a61
Open in Devin Desktop: https://app.devin.ai/desktop/session/919705a00f754ff494dff1d2ed6f9a61?variant=devin