perf: cut gooddata_sdk import cost by importing only what the SDK uses - #1815
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (55)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe SDK now loads public exports and nested submodules lazily, with selected API and model re-exports. SDK consumers use these modules instead of aggregate generated namespaces. Pandas execution paths use a direct response import, and tests cover the import behavior. ChangesSDK lazy import system
Pandas execution response import
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant Caller
participant gooddata_sdk
participant LazyResolver
participant SDKModule
Caller->>gooddata_sdk: Access public export or submodule
gooddata_sdk->>LazyResolver: Resolve missing attribute
LazyResolver->>SDKModule: Import target module
SDKModule-->>gooddata_sdk: Provide object or submodule
gooddata_sdk-->>Caller: Return resolved attribute
Merge Risk: ⚪ Minimal · up to The lazy-import migration preserves the reviewed public import and generated-model contracts, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 50 files. (5 skipped: 1 unsupported, 4 over the file limit.)
A rabbit reads each line, Comment |
hkad98
force-pushed
the
jkd/lazy-sdk-imports
branch
from
September 16, 2026 13:44
9d14c31 to
67d13be
Compare
`import gooddata_sdk` pulled in 2333 modules and ~0.58s on a warm cache, because
three separate layers imported everything available rather than what was needed.
A consumer that wants a single class (e.g. `Visualization`) paid the whole bill.
1. `gooddata_sdk/__init__.py` eagerly re-exported every public name, so touching
any leaf module ran the entire SDK's import graph first. The re-exports are now
resolved lazily via PEP 562 `__getattr__` (the pattern scipy/sklearn use), with
`__all__` for `import *` and a `TYPE_CHECKING` block so type checkers and the
griffe-based docs builder still resolve every name statically.
2. Five modules did `import gooddata_api_client.models as afm_models`, and
`execution.py` did `from gooddata_api_client import models`. That aggregate
imports all 1402 generated model classes; the SDK references 58. The generated
header of that file says as much ("import only the models that you directly
need"), and it is safe because openapi-generator emits a per-model
`lazy_import()`, so leaf imports do not cascade. The 58 now live in
`gooddata_sdk/_models.py`, which keeps the `afm_models.X` spelling at the call
sites and gives a single list to maintain.
3. `client.py` and `catalog_service_base.py` did `from gooddata_api_client import
apis`, importing all ~130 generated API classes (and transitively most models)
to instantiate six. Those six now come from `gooddata_sdk/_apis.py`.
Measured on py3.14, warm .pyc, best of 7 with the two versions interleaved:
import gooddata_sdk 0.58s / 2333 mods -> 0.04s / 160 mods
from gooddata_sdk.visualization import
Visualization 0.54s / 2333 mods -> 0.19s / 991 mods
from gooddata_sdk import GoodDataSdk 0.65s / 2333 mods -> 0.33s / 1402 mods
The module counts are the stable figure; the wall times are this machine and move
with load, so they are interleaved rather than compared across runs.
Keeping attribute access working
--------------------------------
The eager `__init__` used to populate the whole module tree as a side effect, so
`import gooddata_sdk` then `gooddata_sdk.catalog.workspace` resolved. Dropping it
broke that, so every subpackage now gets a lazy submodule `__getattr__` built by
`gooddata_sdk/_lazy.py`. It deliberately does not translate a `ModuleNotFoundError`
raised *inside* an existing submodule into `AttributeError`, so a genuinely missing
dependency still reports itself rather than looking like a typo.
Two intentional differences remain. `dir(gooddata_sdk)` now lists the public API
plus every submodule (previously only the ones that happened to be imported), and
`from gooddata_sdk import *` no longer leaks the `logging` module or the submodule
names, since `__all__` now states the public API explicitly.
Maintaining the lazy-import tables
----------------------------------
PEP 562 needs the name -> module mapping as data, and hand-maintaining ~260 string
pairs would drift from the real imports invisibly: a missing entry means
`from gooddata_sdk import NewThing` raises AttributeError at runtime, and a stale
module path is just a wrong string.
So the `TYPE_CHECKING` block is the single source of truth. It holds ordinary
`from x import Y` statements that IDEs autocomplete, refactorings rewrite and
`ty`/pyright verify, and `scripts/sync_lazy_imports.py` derives `_LAZY_IMPORTS` and
`__all__` from it by AST into a marker-delimited region that is never edited by
hand. Adding a public export is one normal import plus `--update`.
Drift is caught in two independent places, verified against three scenarios (export
added, module renamed, export removed):
- the script with no flags reports drift and exits non-zero, and
`test_lazy_imports_map_matches_type_checking_block` runs it, so CI fails with the
command to fix it;
- `test_all_lazy_exports_resolve` covers the other half -- a module that moved
without the `TYPE_CHECKING` block being updated.
The rest of `tests/sdk/test_lazy_imports.py` guards the module savings themselves,
since one convenience import silently undoes them. The attribute-access cases each
run in their own interpreter: a missing lazy hook in a subpackage is masked as soon
as anything else has imported the tree.
gooddata-pandas used the models aggregate for one class; it now imports it
directly, and the test that patched `gooddata_pandas.dataframe.models.X` patches
`gooddata_pandas.dataframe.X`.
Its `[tool.ty.analysis]` allowed-unresolved-imports needed the `.**` glob that
gooddata-sdk already uses: the bare `gooddata_api_client` entry covered the old
aggregate import but not a leaf `gooddata_api_client.model.*` module.
hkad98
force-pushed
the
jkd/lazy-sdk-imports
branch
from
September 16, 2026 13:50
67d13be to
8125426
Compare
hkad98
enabled auto-merge
September 16, 2026 13:52
no23reason
approved these changes
Sep 16, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1815 +/- ##
==========================================
+ Coverage 82.49% 82.55% +0.05%
==========================================
Files 283 324 +41
Lines 20448 20543 +95
==========================================
+ Hits 16869 16959 +90
- Misses 3579 3584 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
import gooddata_sdkpulls in 2228 modules / ~0.34s on a warm cache. Three layers each import everything available rather than what is needed, so a consumer that wants one class pays the whole bill.import gooddata_sdkfrom gooddata_sdk.visualization import Visualizationfrom gooddata_sdk import GoodDataSdk(whole SDK)Measured on py3.14, warm
.pyc, same interpreter.What was importing too much
1.
gooddata_sdk/__init__.pyeagerly re-exported all 253 public names. Touching any leaf module ran the entire SDK import graph first. Now resolved lazily via PEP 562__getattr__— the pattern scipy and scikit-learn ship — with__all__forimport *and aTYPE_CHECKINGblock so type checkers, IDEs and the griffe docs builder still resolve every name statically.2. The SDK imported all 1402 generated models to use 58. Five modules did
import gooddata_api_client.models as afm_models;execution.pydidfrom gooddata_api_client import models. That aggregate's own generated header says not to do this ("import only the models that you directly need"), and leaf imports are safe because openapi-generator emits a per-modellazy_import(), so they don't cascade. The 58 now live ingooddata_sdk/_models.py, keeping theafm_models.Xspelling at the call sites and leaving one list to maintain.3.
client.pyandcatalog_service_base.pyimported all ~130 generated API classes to instantiate six.from gooddata_api_client import apistransitively pulls in most of the models too — this, not the models aggregate, was the single biggest contributor. The six now come fromgooddata_sdk/_apis.py.Backwards compatibility
from gooddata_sdk import <anything>is unchanged — all 253 exports verified to resolve, and class identity is unchanged (the shims re-export the same objects from the same leaf modules).The eager
__init__also populated the whole module tree as a side effect, soimport gooddata_sdkfollowed bygooddata_sdk.catalog.workspaceused to resolve. Dropping it broke that, so every subpackage gets a lazy submodule__getattr__built bygooddata_sdk/_lazy.py. It deliberately does not convert aModuleNotFoundErrorraised inside an existing submodule intoAttributeError, so a genuinely missing dependency still reports itself instead of looking like a typo.Two intentional differences remain:
dir(gooddata_sdk)now lists the public API plus every submodule, where before it listed only those that happened to have been imported.from gooddata_sdk import *no longer leaks theloggingmodule or the submodule names (catalog,utils, …), because__all__now states the public API explicitly. Star-importing a library is not a supported pattern and this fails loudly withNameErrorrather than silently.Verification
test_catalog_user_service.pyerrors on this branch are pre-existing and reproduce identically on unmodifiedmaster; they come from a live server returning aDENODOdata-source type the checked-in generated client does not know.ruff check/formatclean;tyreports the same 5 pre-existing diagnostics asmaster.python_ref_builder.pyfilters out.tests/sdk/test_lazy_imports.pyguards all three properties. A single convenience import silently undoes the module savings, and a missing lazy hook in a subpackage only surfaces in a fresh interpreter — so the attribute-access cases each run in their own subprocess. (My first pass shared one interpreter and the regression hid behind an earlier import.)Summary by CodeRabbit
Performance
Compatibility
Tests