Studio: keep pandas out of the backend startup import graph - #8962
Conversation
Importing data_designer_unstructured_seed.chunking runs the package __init__ first, and that re-exports .config and .impl, which import the data designer engine, which imports pandas and pyarrow. Dropping the module-scope import inside chunking.py therefore left the whole cost in import main's graph wherever the plugin is installed: measured 534ms cumulative for routes.data_recipe.seed, with pandas and pyarrow both loaded. The route resolves the plugin through _chunking() on first use instead. None still means "not installed", so the existing 500 and the raw-text fallback are unchanged, and the failed probe is remembered rather than retried per request. The runtime guards in test_startup_defers_torch.py pass vacuously wherever the optional plugin is not installed, which is most CI jobs, so add a source-level guard that reads seed.py and fails on a module-scope import of the plugin either way. test_data_recipe_seed.py covers the availability cases: unavailable without the plugin on both preview entry points, probed once, raw text fallback, and the installed path.
for more information, see https://pre-commit.ci
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
💡 Codex Review Offload the cold plugin import from the async upload route
When the plugin is installed and the first unstructured upload arrives, ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Confirmed the mechanism: That call site is not from this change. Cold, in a fresh interpreter: pandas 258ms, pyarrow 89ms, pymupdf4llm 91ms. The one-time plugin resolution is the same order as work the handler already does synchronously, and it replaces the 2.247s that every launch paid before uvicorn bound the port, whether or not the user ever opens unstructured seed. Offloading the handler is worth doing, but it should cover the write, the directory scan and the conversion rather than the import alone, and it changes the route's concurrency, which this PR deliberately leaves alone. Keeping it separate. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 524033d9f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| raise ValueError(f"Unsupported file type: {ext}") | ||
|
|
||
| if normalize_unstructured_text is None: | ||
| chunking = _chunking() |
There was a problem hiding this comment.
On the first unstructured upload, the async def upload_unstructured_file handler calls _extract_text_from_file synchronously, so this newly deferred _chunking() call imports the plugin, pandas, and pyarrow directly on uvicorn's event-loop thread. For a first .txt or .md upload, the documented multi-second import delay therefore stalls every concurrent request instead of merely delaying startup; resolve the plugin in a thread pool or otherwise keep the cold import outside the async handler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thumbs down. The code observation is right and the impact does not reach the bar, and this is also a verbatim re-raise of a point already settled on this PR.
Settled already. The identical claim was posted as a top-level review on 50a70bb226 and answered there, after which the review round closed with "Didn't find any major issues" on that same commit. The only commits since are 2184ab73f, a merge from main, and 524033d9f, a pre-commit.ci pass touching only test_lan_access_settings.py, test_compile_caches_are_per_worker.py and test_no_test_shadows_another.py. seed.py:427 is byte-identical to the round that was already closed.
On the substance. Yes, the cold probe now runs on the loop. It is a sub-second one-shot in an async def handler that already blocks the loop longer, on pre-existing code this PR does not touch. Measured on Linux, py3.13, with the plugin and data-designer-engine installed, n=3 each:
| merge base | head | |
|---|---|---|
first .txt upload, async def handler |
0.0006 s | 0.541 s |
| event-loop stall on that upload | 0.0056 s (idle baseline 0.0053 s) | 0.546 s, once per process |
| every subsequent upload | 0.00045 s | 0.00050 s |
import pymupdf4llm, already on that same loop on the merge base |
1.027 s | 1.027 s |
| extracted text | normalized | normalized, identical |
Once per process, and half the cost of an import that has always sat on that loop ahead of it. No crash, no wrong result, no data loss.
Reachability, since only one of the three call sites is on the loop. _read_preview_rows_from_unstructured_file and _read_preview_rows_from_multi_files are reached only from inspect_seed_upload, a sync def route, which FastAPI dispatches to the threadpool. Only _extract_text_from_file is on the loop, via upload_unstructured_file, async def since dd283b0605 (#4468), which is where the direct call was already made.
On the remedy. asyncio.to_thread, or making the handler sync, would also move raw_path.write_bytes, _get_block_total_size and the pdf and docx conversion off the loop. That changes the route's concurrency for pre-existing work and is a separate change, correctly out of scope here.
|
@codex review |
|
On the red check: The failing assertion is
|
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Gate 1: is this solving a real problem, and does it break anythingVerified independently rather than taking the PR body's word for it, on Linux, py3.13, importing the seed route the way
So the problem is real, and it is worse than the PR body claims. On the merge base the module-scope try:
from data_designer_unstructured_seed.chunking import (...)
except ImportError:
...fails in an environment without the Does it break the old pathNo. Both import forms behave identically on both trees, checked in fresh interpreters, which matters here: Worth recording the trap, because it produced a false regression report on the way: run in one interpreter, the package-form import fails first and leaves enough behind that the submodule form then appears to succeed and normalize. In a fresh process it does not. Any conclusion about this module drawn from a reused interpreter is an artifact. Stated gaps, not implied coverageThe |
2 participants
import mainpulls pandas on every Studio startup, through a route that never needs it at import time.The Startup profile workflow shows the chain outright (run 31930823568, windows-latest):
routes/data_recipe/seed.pyimports the unstructured seed plugin at module scope to decide whether unstructured seed support is available. uvicorn binds the socket only afterimport mainfinishes, so this is time the login screen does not exist.import mainChange
seed.pyresolves the plugin through a_chunking()helper on first use.Nonestill means "not installed", so the existing 500 and the raw-text fallback are unchanged.chunking.pyresolves pandas through a_pandas()helper instead of at module scope. Two of the four call sites already did their own guardedimport pandas as pdwith the same error message; those fold into the helper, so "pandas is required for unstructured seed processing" is written once and covers all four.The route is the part that matters. Importing
data_designer_unstructured_seed.chunkingruns the package__init__first, and that re-exports.configand.impl, which import the data designer engine, which imports pandas and pyarrow. So taking the import out ofchunking.pyalone leaves the whole cost in the graph on any install that actually has the plugin.Measured with the plugin installed,
python -X importtime -c "import main":routes.data_recipe.seedcumulativeimport mainpandas and pyarrow no longer appear in the graph at all.
Behaviour
Unchanged in every case, checked in isolated venvs against
main, not just against the previous commit here:Unstructured seed support not availableNothing about chunking, caching, parquet layout or the preview rows changes. A failed probe is remembered, so a preview call on an install without the plugin is not a fresh import attempt every time, which is what the module-scope import gave before.
Coverage
tests/test_startup_defers_torch.pyalready holds the "import mainmust not import heavy modules" invariant in a fresh interpreter. This addspandasandpyarrowto that list androutes.data_recipe.seedto the per-module guard.Those two are runtime guards, and they pass vacuously wherever the optional plugin is not installed, which is most CI jobs: the route falls back to "unavailable" and imports nothing. So there is also a source-level guard that parses
seed.pyand fails on a module-scope import of the plugin, which holds in either environment.tests/test_data_recipe_seed.pycovers the availability semantics: unavailable without the plugin on both preview entry points, probed once rather than per call, raw text fallback for the extractor, and the installed path still normalizing through the plugin.Repeat calls
_pandas()holds the resolved module in a global rather than re-running theimportstatement, measured over 2,000,000 calls with pandas already in
sys.modules:import pandas as pdinside the functionNonecheckWorth having here because it costs one branch, but it is worth being clear about the
scale: 36 ns against the 250 ms this same import costs the first time it runs on this
machine, and 1,325 ms in CI on Windows. The saving that matters in this PR is the cold
one, on startup. There are about 3,250 other function-level lazy imports in the backend
and converting them would be a large diff for a per-call saving that is below noise at
every one of those call sites, so this PR does not touch them.
Testing
import mainis pandas-free and pyarrow-free in both; the missing-plugin and missing-pandas paths return the same 500 they returned onmain.build_unstructured_preview_rowsandmaterialize_multi_file_unstructured_seed)._pandas()with pandas blocked: sameRuntimeError, the global is not left poisoned, a later import recovers in the same process. 16 threads racing it all get one module object.pytest tests/test_data_recipe_seed.py tests/test_startup_defers_torch.py tests/test_refactor_guard.py tests/test_text_io_encoding.py,ruff checkon the changed files,git diff --check.The Startup profile workflow runs on this path, so the three-platform numbers will be reported on this PR directly.