Skip to content

fix(imports): lazy package exports — no import cycle, no Arelle or rdflib for callers that don't need them - #74

Merged
jfrench9 merged 2 commits into
mainfrom
bugfix/periods-circular-import
Sep 25, 2026
Merged

jfrench9 merged 2 commits into
mainfrom
bugfix/periods-circular-import

Conversation

@jfrench9

@jfrench9 jfrench9 commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Summary

import xbrlkit.periods as a first import fails on 0.18.1 with a circular-import ImportError. periods imports xbrlkit.parse.ids; importing any parse submodule runs parse/__init__.py, which eagerly imported arelle_load and to_model, and to_model imports periods while it is still half-loaded. The usual entry points import in an order that hides it, but periods is a public module.

Changes

  • xbrlkit/parse/__init__.py: the eight exports (load_model, to_xbrl_model, close, …) resolve on first use through a module __getattr__ (PEP 562), with the imports kept under TYPE_CHECKING for type checkers. from xbrlkit.parse import to_xbrl_model works as before. The cycle is gone, and a caller that only wants a period id (xbrlkit.periods) no longer loads Arelle.
  • xbrlkit/serialize/__init__.py, xbrlkit/deserialize/__init__.py: the same lazy pattern. Both packages imported every projection eagerly, so importing xbrlkit.serialize.tavi or xbrlkit.deserialize.tavi loaded rdflib through the holon and graph modules. TAVI is JSON; it now loads no RDF stack. Public names are unchanged.
  • tests/test_imports.py: every public module imported in its own interpreter (48 modules), plus checks that xbrlkit.periods does not load Arelle and that the two TAVI modules do not load rdflib. Against 0.18.1, exactly xbrlkit.periods and the Arelle check fail.

Output Impact

INTERNAL. No emitted file and no CLI command changes; xbrlkit.parse's exports are the same names.

Testing

  • just test-all: 603 passed, 2 skipped; ruff, format and basedpyright clean.
  • Reproduced first: uvx --from xbrlkit==0.18.1 python -c "import xbrlkit.periods" raises the ImportError.

🤖 Generated with Claude Code

`import xbrlkit.periods` as a first import failed in 0.18.1: periods imports
xbrlkit.parse.ids, importing any parse submodule runs parse/__init__.py,
and that eagerly imported arelle_load and to_model, which imports periods
while it is half-loaded. parse/__init__.py now resolves its exports on first
use (PEP 562), so the cycle is gone and a caller that only wants a period
id no longer loads Arelle.

tests/test_imports.py imports every public module in its own interpreter,
so an order-dependent import fails CI instead of a user.
@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Claude finished @jfrench9's task in 2m 41s —— View job


Review: PR #74 — fix(parse): load the Arelle-backed exports lazily

  • Gather context (diff, related modules, existing usages)
  • Understand the request
  • Review xbrlkit/parse/__init__.py changes
  • Review tests/test_imports.py
  • Post review feedback

Summary

Confirmed the root cause and the fix. The chain is real: xbrlkit/periods.py:21 imports xbrlkit.parse.ids, which runs xbrlkit/parse/__init__.py first; that file used to eagerly import xbrlkit.parse.to_model, and to_model.py:40 imports xbrlkit.periods — a genuine cycle that only breaks by accident depending on import order. import xbrlkit.periods as a first import is a legitimate, minimal repro of that.

xbrlkit/parse/__init__.py

  • The PEP 562 __getattr__ + _LAZY dict approach is correct and idiomatic. Moving the real imports under TYPE_CHECKING keeps static typing intact (basedpyright resolves the real symbol, not Any) while removing the runtime import.
  • Verified this doesn't break existing call sites that rely on the public surface: xbrlkit/serve/session.py:1092 (from xbrlkit.parse import close, load_model, to_xbrl_model), tests/test_parse.py:25 (deferred from xbrlkit.parse import (...)), and xbrlkit/parse/arelle_load.py:242 (from xbrlkit.parse import register_sec_transforms) — all go through normal attribute resolution / __getattr__ and work unchanged. from xbrlkit.parse import * also still works since __all__ is intact and import * does getattr per name.
  • xbrlkit/parse/arelle_load.py:13 still does from xbrlkit.parse import ids, but by the time __getattr__ triggers a load of arelle_load, xbrlkit.parse.__init__ has already finished executing, so this is a plain (non-circular) submodule import — no residual cycle.
  • Minor, non-blocking observation: __all__ and _LAZY's keys are two hand-maintained lists that must stay in sync (currently they do). Not worth a helper for 8 names, just flagging it as the one thing to remember on the next export addition/removal.

tests/test_imports.py

  • Good design: importing every public module in its own subprocess is the right call here specifically because the bug is import-order dependence — a single shared interpreter would hide it once anything else had already warmed up sys.modules.
  • _public_modules correctly filters out _-prefixed path segments, so xbrlkit._vendor.* and xbrlkit.serialize._kernel.*/_values are excluded, matching the "48 modules" count in the PR description.
  • test_periods_does_not_load_arelle is a nice, direct regression guard for the actual behavioral guarantee (not just "it imports," but "it stays cheap").

Verification

  • CI is green: both the Test and PR #74 workflow runs completed successfully.
  • I wasn't able to execute python/pytest directly in this sandbox (commands requiring code execution were blocked pending approval), so I relied on static review plus the passing CI run rather than re-running the suite myself.

No bugs found. The fix is minimal, targets the actual cycle, and the new test would have caught the regression this PR is fixing. Approving from a code-review standpoint (note: I can't submit a formal GitHub approval).

@jfrench9 jfrench9 changed the title fix(parse): load the Arelle-backed exports lazily fix(imports): lazy package exports — no import cycle, no Arelle or rdflib for callers that don't need them Sep 25, 2026
@jfrench9
jfrench9 merged commit f4a50ec into main Sep 25, 2026
4 checks passed
@jfrench9
jfrench9 deleted the bugfix/periods-circular-import branch September 25, 2026 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant