Repository navigation
Improve import startup with lazy top-level exports - #2950
fede-kamel wants to merge 1 commit into
Conversation
|
Validation summary for #2819:
Interpretation: default lazy mode reduces import startup work significantly; eager mode remains available for CI/dev verification. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bd0b97246
ℹ️ 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".
|
Two things, about the env var for eager load
I think it's great you're working on a fix for this btw... thank you |
|
Took this feedback and removed the runtime eager-import env var path from The branch now keeps lazy imports as the only product behavior and makes the coverage explicit in tests instead:
Validated locally with:
|
|
@RobertCraigie @rachellim @g0t4 — Friendly ping! Just re-validated the branch: all tests pass ( |
2cb445c to
b0fb0f4
Compare
|
Friendly bump for maintainer review — rebased onto What changed since last review
Integration benchmark (Python 3.13, cold
|
| metric | origin/main (2.32.0) |
this branch | speedup |
|---|---|---|---|
| min | 346 ms | 288 ms | 1.20× |
| median | 401 ms | 325 ms | 1.23× |
| mean | 397 ms | 338 ms | 1.18× |
Python's own -X importtime cumulative time for the openai module:
origin/main: 390 ms- this branch: 232 ms → 1.68× faster / ~158 ms shaved
Repro: python scripts/bench_import.py (also in the branch).
What still stays lazy after the rebase
Verified on import openai:
openai.lib.azure— not loadedopenai.lib.streaming— not loadedopenai.lib._tools— not loaded
These are the biggest non-transitively-required submodules, which is where the ~160 ms comes from.
Happy to split further or squash to the original 3 commits if preferred. Would love to get this in — the import-time win is meaningful for CLI / Lambda / short-lived workers.
|
@mcgrew-oai @hintz-openai — friendly ping in case this slipped off the review queue. Just rebased onto 2.32.0 (see benchmark comment above: ~1.18–1.23× faster cold |
|
Friendly bump for Latest benchmark on the rebased branch (Python 3.13, cold
No API surface change — Happy to split commits, narrow scope, or close and reopen with smaller diff if it'd help review velocity. |
b0fb0f4 to
66532e1
Compare
|
@fede-kamel you'd think maybe @codex could review/fix/merge this one... after all, we're getting replaced any day now per Dario |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66532e1c09
ℹ️ 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".
| from ._parsing import ResponseFormatT as ResponseFormatT | ||
|
|
||
|
|
||
| def __getattr__(name: str) -> Any: |
There was a problem hiding this comment.
The lazy conversion drops pydantic_function_tool and ResponseFormatT from from openai.lib import * because these names now exist only behind __getattr__, and wildcard imports do not call __getattr__ unless __all__ is defined. Before this change both symbols were bound at module import time, so code relying on wildcard imports will now silently miss them.
Useful? React with 👍 / 👎.
| globals()[name] = value | ||
| return value | ||
|
|
||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
This AttributeError now interpolates __name__, but earlier in the module a loop assigns for __name in __all__, so __name__ is no longer 'openai' by the time this runs. As a result, unknown-attribute failures can report the wrong module name, which makes debugging and import error messages misleading.
Useful? React with 👍 / 👎.
66532e1 to
0c6ff1a
Compare
|
@apcha-oai @hintz-openai — bumping for a look when you have a moment. Status: Rebased cleanly onto
Benchmarks on the rebased branch (Linux/WSL, Python 3.12, 10 cold imports each):
Modules deferred at import time on the lazy path vs. baseline:
Tests: Genuine question: is there a reason not to take a free import-time speedup that's behind an opt-in flag for safety? Happy to adjust the surface or split the change if there's a smaller slice that would be easier to land. |
0c6ff1a to
de6738b
Compare
|
Going to skip the standard ping and ask a structural question instead, since this PR has been open ~10 weeks across 5+ reviewer pings without engagement and I want to make sure I'm asking for the right thing. The structural concern: Empirical counter-evidence: just rebased The honest question for maintainers: what's the path you'd actually want here? Three reasonable options I can see:
I'm happy with any of these — option (3) is the least work for you and probably the right answer if the SDK team's priorities don't include hand-maintained perf patches. Could one of you ( No-pressure framing: "close it, file the issue" is a totally fine answer. |
de6738b to
538cd54
Compare
Defer azure, bedrock, streaming, pydantic_function_tool and websocket reconnection exports behind a module __getattr__ so that `import openai` no longer eagerly imports boto3 (bedrock) or the streaming stack. OPENAI_EAGER_IMPORT=1 resolves them up-front to surface import errors.
538cd54 to
bb25387
Compare
|
Quick data point for this change: on current |
|
@g0t4 @apcha-oai @hintz-openai — closing this import-startup experiment. The benchmark and issue evidence remain useful, but the relevant import surface is generated and there has been no maintainer direction on whether the work belongs in generator templates or as a manual patch. Rather than leave a stale change against a generated file open, I am withdrawing it. Thank you for the thoughtful early feedback. |
Summary
This PR addresses import-time startup regression tracked in #2819 by making heavy top-level exports lazy.
Key changes:
openai.__init__for:AzureOpenAIAsyncAzureOpenAIpydantic_function_toolAssistantEventHandlerAsyncAssistantEventHandlerOPENAI_EAGER_IMPORT=1scripts/bench_import.py.tests/test_import_surface.pytests/lib/test_import_surface_live.py(live-gated)Why this relates to #2819
Issue #2819 reports that
import openaiis too slow and that eager loading of type-heavy/internal surfaces is a major contributor. This PR reduces startup cost in the default path by moving expensive imports behind first-use access while preserving API compatibility.Testing done
Unit / behavior tests
PYTHONPATH=src python3 -m pytest tests/test_import_surface.py -q -o addopts=''-> passedPYTHONPATH=src python3 -m pytest tests/test_module_client.py -q -o addopts=''-> passedLive API-key integration test
OPENAI_LIVE=1 PYTHONPATH=src OPENAI_API_KEY=... python3 -m pytest tests/lib/test_import_surface_live.py -q -o addopts=''-> passedclient.models.list()).Performance benchmarks
Using
scripts/bench_import.py:0.1533sOPENAI_EAGER_IMPORT=1):0.2601s+0.1068s(+69.7%)0.7019s2.9429s+2.2410s(+319.3%)Interpretation: eager mode is intentionally slower, and the default lazy path materially reduces startup work compared with forced eager loading.
Notes
OPENAI_LIVE=1andOPENAI_API_KEY.