Skip to content

fix: defer pandas import in ResponseToFileExtractor - #1221

Merged
Patrick Nilan (pnilan) merged 3 commits into
mainfrom
devin/1791494270-defer-pandas-import
Oct 21, 2026
Merged

Patrick Nilan (pnilan) merged 3 commits into
mainfrom
devin/1791494270-defer-pandas-import

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

response_to_file_extractor.py imported 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 in ResponseToFileExtractor._read_with_chunks.

- import pandas as pd   # module level
  def _read_with_chunks(self, path, file_encoding, chunk_size=100):
+     try:
+         import pandas as pd   # before the main `try:` so `except pd.errors.EmptyDataError` resolves
+     except ImportError:
+         os.remove(path)       # don't leave the downloaded response on disk
+         raise
      try:
          ...

The CDK already uses the same lazy-import approach for unstructured in file_based/file_types/unstructured_parser.py. Behavior is unchanged, including preserve_na_values. The only consumer in the monorepo is source-salesforce, a Python connector. Its tests patch source_salesforce.streams.ResponseToFileExtractor, not the module's pd.

Measured locally:

  • On main, importing airbyte_cdk.cli.source_declarative_manifest._run leaves both pandas and numpy in sys.modules (True True). -X importtime attributes about 198 ms cumulative to response_to_file_extractor.
  • On this branch it prints False False, and the cumulative import time is about 0.4 ms.

Other changes:

  • Dockerfile: adds RUN 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 the limit_memory("20 MB") test would count the lazy import, about 44 MiB, when run alone.

Trade-off: with a broken pandas install, check now passes, and only CSV-extractor streams fail at read time, with ImportError. 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 check for manifest-only connectors).

Test Coverage

  • New test, unit_tests/source_declarative_manifest/test_source_declarative_manifest_imports.py:
    • Runs SDM spec, check (HTTP mocked with requests_mock) and discover on the PokeAPI test manifest in a subprocess, with cwd pinned to the repo root.
    • Asserts that neither pandas nor numpy was imported.
    • Fails without the fix (["numpy", "pandas"]) and passes with it.
  • Existing extractor tests still run pd.read_csv, including preserve_na_values. The memory test passes when run alone.
  • Locally: unit_tests/source_declarative_manifest/, the extractor tests and unit_tests/sources/declarative/requesters/ pass. ruff and mypy are clean.
  • Lazy-import guard: a manual check with pandas made unimportable confirmed the ImportError propagates and the temp file is deleted.
  • source-salesforce: per the review, its unit tests passed 159/159 against this branch's CDK, including 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

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You 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-import

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

PyTest Results (Fast)

5 839 tests  +1   5 808 ✅ +1   12m 21s ⏱️ + 2m 26s
    1 suites ±0      31 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 5d732de. ± Comparison against base commit 693f39a.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

PyTest Results (Full)

5 842 tests  +1   5 811 ✅ +1   10m 58s ⏱️ - 6m 3s
    1 suites ±0      31 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 5d732de. ± Comparison against base commit 693f39a.

♻️ This comment has been updated with latest results.

@pnilan Patrick Nilan (pnilan) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 title check 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_mock

Recommended

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 of ImportError.

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)
    raise

If 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:18 ends 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:

  1. Mock a small CSV download.
  2. Run ResponseToFileExtractor on it.
  3. 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_cdk than 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 bare StopIteration. Use next(..., None) and assert with result.stdout and result.stderr in the message.
    • Do the same for the asserts at lines 55-56, because the subprocess exits 0 even when check fails.
  • Fixtures. Take the conftest fixtures valid_local_manifest and valid_local_config_file as 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.py 23/23. Please record this or equivalent evidence.
  • Trade-off note, if #3 isn't applied: with a broken pandas install, check now 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 pd alias isn't referenced in this repo, in the connector monorepo, or anywhere an org-wide code search finds.
  • CSV output. Byte-identical to main across 60 cases, covering preserve_na_values on 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 main with ['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-54 lists perf, refactor, style, test and revert as allowed types, but the title check rejects all five. That is probably where the perf: title came from.

@devin-ai-integration devin-ai-integration Bot changed the title perf: defer pandas import in ResponseToFileExtractor fix: defer pandas import in ResponseToFileExtractor Oct 21, 2026
Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Thanks Patrick Nilan (@pnilan). I've addressed all six items in 5d732de.

  1. Title: retitled to fix: defer pandas import in ResponseToFileExtractor.
  2. Memory test: test_response_to_file_extractor.py now imports pandas at module level. The memory test passes when run alone (-m slow).
  3. Temp-file cleanup: the lazy import is wrapped in try/except ImportError that runs os.remove(path) and re-raises. It stays outside the main try:. With pandas made unimportable, the ImportError propagates and the temp file is deleted.
  4. SDM image: added RUN python -c "import pandas" after the wheel install in Dockerfile. I skipped the optional CSV extraction in the subprocess test, because the extractor unit tests already cover that parse path.
  5. Subprocess directory: subprocess.run now uses cwd=Path(__file__).resolve().parents[2]. The test passes when invoked from /tmp.
  6. Nits: added the docstring. Every assert now includes stdout and stderr in its message, and the marker lookup uses next(..., None). The test takes the valid_local_manifest and valid_local_config_file fixtures.

The PR body now includes your source-salesforce run (159/159, with test_bulk_stream.py 23/23) and the updated trade-off note.

Written by Devin

@pnilan
Patrick Nilan (pnilan) marked this pull request as ready for review October 21, 2026 20:28
@pnilan
Patrick Nilan (pnilan) requested a review from a team as a code owner October 21, 2026 20:28
@pnilan
Patrick Nilan (pnilan) merged commit b8ccd9d into main Oct 21, 2026
30 of 31 checks passed
@pnilan
Patrick Nilan (pnilan) deleted the devin/1791494270-defer-pandas-import branch October 21, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant