Cyclic imports

[DimitriPapadopoulos]

Zarr version

v3

Description

The second cycle is already identified as such:

• zarr.api.asynchronous ⟷ zarr.core.group

  from zarr.core.group import AsyncGroup, ConsolidatedMetadata, GroupMetadata

  import zarr.api.asynchronous as async_api
  

• zarr.storage.common -> zarr.storage.remote

         from zarr.storage.remote import RemoteStore  # circular import

  from zarr.storage.common import _dereference_path

Steps to reproduce

https://app.deepsource.com/gh/DimitriPapadopoulos/zarr-python/issue/PYL-R0401/occurrences

[maxrjones]

Took a fresh look at this against main (post-3.2.1).

Cycle 1 — zarr.api.asynchronous ↔ zarr.core.group: still present. Both edges are top-level:

• src/zarr/core/group.py:17 — import zarr.api.asynchronous as async_api
• src/zarr/api/asynchronous.py:34 — from zarr.core.group import AsyncGroup, ConsolidatedMetadata, GroupMetadata, create_hierarchy

Cycle 2 — zarr.storage.common ↔ zarr.storage.remote: fixed. The modules were renamed to zarr.storage._common / zarr.storage._fsspec, and the back-edge is now a function-local deferred import (src/zarr/storage/_common.py:331, inside make_store_path). _fsspec.py no longer imports _common at all.

Bonus finding: there's also a top-level zarr ↔ zarr.core.array / zarr.core.chunk_grids cycle — each does import zarr at module level. Python tolerates it because zarr is in sys.modules before __init__.py finishes, but it's still a real cycle. Probably worth its own issue; out of scope here.

How to reproduce

Save as check_cycles.py and run with uv run --no-project check_cycles.py from the repo root. Stdlib-only, ~150 lines, AST + Tarjan SCC over top-level import/from statements (deferred imports inside functions or TYPE_CHECKING are correctly ignored).

"""Detect top-level (eager) import cycles in zarr.* using AST + Tarjan."""
# /// script
# requires-python = ">=3.11"
# ///
from __future__ import annotations

import ast
import pathlib
import sys
from collections import defaultdict


def module_name_from_path(root: pathlib.Path, file: pathlib.Path) -> str:
    rel = file.relative_to(root).with_suffix("")
    parts = list(rel.parts)
    if parts[-1] == "__init__":
        parts = parts[:-1]
    return ".".join(parts)


def all_modules(src: pathlib.Path, pkg_root: pathlib.Path) -> set[str]:
    mods: set[str] = set()
    for py in src.rglob("*.py"):
        if "__pycache__" in py.parts:
            continue
        mods.add(module_name_from_path(pkg_root, py))
    return mods


def resolve_target(name: str, modules: set[str]) -> str | None:
    if name in modules:
        return name
    parts = name.split(".")
    while len(parts) > 1:
        parts.pop()
        candidate = ".".join(parts)
        if candidate in modules:
            return candidate
    return None


def collect_top_level_imports(tree: ast.AST, modules: set[str]) -> set[str]:
    edges: set[str] = set()
    for node in tree.body:
        if isinstance(node, ast.Import):
            for n in node.names:
                if n.name.startswith("zarr"):
                    tgt = resolve_target(n.name, modules)
                    if tgt:
                        edges.add(tgt)
        elif isinstance(node, ast.ImportFrom):
            if node.module and node.module.startswith("zarr"):
                added_specific = False
                for n in node.names:
                    tgt = resolve_target(f"{node.module}.{n.name}", modules)
                    if tgt and tgt != node.module:
                        edges.add(tgt)
                        added_specific = True
                if not added_specific:
                    tgt = resolve_target(node.module, modules)
                    if tgt:
                        edges.add(tgt)
    return edges


def main() -> None:
    src = pathlib.Path("src/zarr")
    pkg_root = src.parent
    modules = all_modules(src, pkg_root)

    graph: dict[str, set[str]] = defaultdict(set)
    for py in src.rglob("*.py"):
        if "__pycache__" in py.parts:
            continue
        mod = module_name_from_path(pkg_root, py)
        tree = ast.parse(py.read_text())
        for tgt in collect_top_level_imports(tree, modules):
            if tgt != mod:
                graph[mod].add(tgt)

    sys.setrecursionlimit(10_000)
    index_counter = [0]
    stack: list[str] = []
    lowlinks: dict[str, int] = {}
    index: dict[str, int] = {}
    on_stack: dict[str, bool] = {}
    sccs: list[list[str]] = []

    def strongconnect(v: str) -> None:
        index[v] = lowlinks[v] = index_counter[0]
        index_counter[0] += 1
        stack.append(v)
        on_stack[v] = True
        for w in graph.get(v, ()):
            if w not in index:
                strongconnect(w)
                lowlinks[v] = min(lowlinks[v], lowlinks[w])
            elif on_stack.get(w):
                lowlinks[v] = min(lowlinks[v], index[w])
        if lowlinks[v] == index[v]:
            comp: list[str] = []
            while True:
                w = stack.pop()
                on_stack[w] = False
                comp.append(w)
                if w == v:
                    break
            sccs.append(comp)

    for v in set(graph) | {t for ts in graph.values() for t in ts}:
        if v not in index:
            strongconnect(v)

    cycles = [c for c in sccs if len(c) > 1]
    if not cycles:
        print("No top-level import cycles detected.")
        return
    for c in cycles:
        print("Cycle:")
        for m in sorted(c):
            print(f"  {m}  ->  {sorted(t for t in graph[m] if t in c)}")


if __name__ == "__main__":
    main()

Suggested fix scope

Two halves:

1. Break the cycle in core/group.py. Drop the top-level import zarr.api.asynchronous as async_api and move it into each of the ~13 method bodies that actually call async_api.X(...) — same per-method deferred-import pattern already used in src/zarr/core/array.py:1192, :1230, :1451. The four type-annotation uses (data: async_api.ArrayLike) need no change because the file already has from __future__ import annotations.
2. Add a regression test. Design left to the contributor — static AST scan, fresh-interpreter import probe, or a hybrid, with whatever allowlist / failure-message ergonomics they prefer. The script above is a forensic tool, not a template.

[d-v-b]

imo we should be open to a more basic structural reorganization instead of pushing imports into function bodies

[maxrjones]

imo we should be open to a more basic structural reorganization instead of pushing imports into function bodies

can you be more specific here? I'm trying to triage issues to see if any would be good for onboarding developers (e.g., @chuckwondo) so it'd be helpful if you could provide more specific direction

[d-v-b]

cyclic imports are a symptom of problems in the design of the modules in a package. I'm not sure what the basic fix is for the cyclic imports flagged here, but the fix is to re-organize code, and that might be tricky as an onboarding task