Skip to content

Add absolute named import support to lazy_export#5078

Open
ooctipus wants to merge 1 commit intoisaac-sim:developfrom
ooctipus:zhengyuz/lazy-export-absolute-named-imports
Open

Add absolute named import support to lazy_export#5078
ooctipus wants to merge 1 commit intoisaac-sim:developfrom
ooctipus:zhengyuz/lazy-export-absolute-named-imports

Conversation

@ooctipus
Copy link
Collaborator

@ooctipus ooctipus commented Mar 21, 2026

Description

Extend _parse_stub and lazy_export to handle absolute named imports
(e.g. from isaaclab.envs.mdp import foo, bar) in .pyi stubs. Previously only
relative imports and absolute wildcard fallbacks were supported. Explicit names from
absolute packages are now eagerly resolved and re-exported at lazy_export() time.

Type of change

  • Bug Fix

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

Extend _parse_stub and lazy_export to handle absolute named imports
(e.g. `from isaaclab.envs.mdp import foo, bar`) in .pyi stubs. These
are eagerly resolved and re-exported alongside the existing relative
and wildcard import paths.

Add unit tests for _parse_stub's new absolute_named return value
covering single/multiple packages, accumulation across lines, mutual
exclusion with wildcards and relative imports, and filtered stub
integrity.
@github-actions github-actions bot added the isaac-lab Related to Isaac Lab team label Mar 21, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 21, 2026

Greptile Summary

This PR extends _parse_stub and lazy_export in source/isaaclab/isaaclab/utils/module.py to handle a previously unsupported stub pattern: absolute named imports (from isaaclab.envs.mdp import foo, bar). The change is additive and non-breaking — the return type of the private _parse_stub helper gains a fourth element (absolute_named: dict[str, list[str]]), and lazy_export eagerly resolves those names at import time via importlib, mirroring the existing eager resolution for relative-wildcard imports.

Key points:

  • The filtered stub written to disk for lazy_loader correctly excludes the new absolute named imports (they are not added to filtered_body), so no regression to the existing lazy-load path.
  • A P1 bug exists: alias.asname is never consulted when building absolute_named. A stub containing from pkg import foo as bar will eagerly publish foo (not bar) into the module namespace, silently exporting the wrong name.
  • Error reporting for the new path is inconsistent with the existing fallback_packages path: a missing package or attribute raises a bare ModuleNotFoundError/AttributeError without a message pointing back to the stub declaration.
  • The new test class TestParseStubAbsoluteNamed provides good unit coverage of _parse_stub routing, but lacks a test for the as-alias edge case.

Confidence Score: 4/5

  • Safe to merge after addressing the alias-handling bug; all other issues are non-blocking style improvements.
  • The implementation is clean and well-tested for the common case. The P1 alias bug (alias.asname ignored) is a real correctness issue, but it only manifests for stub lines of the form from pkg import foo as bar, which do not appear to exist anywhere in the current codebase. Fixing it before merge is the right call, but it carries zero regression risk for existing users. The two P2 error-context gaps are purely cosmetic relative to the fallback_packages pattern. Once the alias fix lands, the PR is straightforwardly ready.
  • source/isaaclab/isaaclab/utils/module.py — the alias-handling logic at lines 68–69 and the missing error wrapping at lines 155–160.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/utils/module.py Core implementation of the new feature. Extends _parse_stub to collect absolute named imports into an absolute_named dict, and eagerly resolves them in lazy_export. Two issues: import aliases (as syntax) are silently dropped (wrong name exported), and missing error-context wrapping around the new import/getattr calls.
source/isaaclab_tasks/test/test_lazy_export_stubs.py Good set of unit tests covering the new _parse_stub return value: single/multiple absolute named imports, accumulation across lines, separation from wildcard/relative cases, mixed stubs, and the filtered-stub exclusion assertion. No tests for the as-alias edge case.
source/isaaclab/config/extension.toml Version bump from 4.5.22 to 4.5.23, consistent with the new changelog entry.
source/isaaclab/docs/CHANGELOG.rst New changelog entry for 4.5.23 correctly describes the absolute named import feature.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["lazy_export() called"] --> B{has .pyi stub?}
    B -- No --> C["__all__ = []"]
    B -- Yes --> D["_parse_stub(stub_file)"]

    D --> E["AST walk over stub body"]
    E --> F{node is ImportFrom?}
    F -- No --> G["add to filtered_body"]
    F -- Yes --> H{classify import}

    H -- "level==1, not star" --> I["add to filtered_body\n(relative named → lazy_loader)"]
    H -- "level==0, star" --> J["fallback_packages.append(module)"]
    H -- "level==1, star" --> K["relative_wildcards.append(module)"]
    H -- "level==0, not star" --> L["absolute_named[module].extend(names)"]
    H -- "level>1 or no module" --> M["silently dropped, needs_filter=True"]

    I --> N["write filtered .pyi\nfor lazy_loader"]
    J --> N
    K --> N
    L --> N

    N --> O["lazy.attach_stub → stub_getattr, stub_dir, __all__"]
    C --> P["mod = sys.modules[package_name]"]
    O --> P

    P --> Q["Resolve absolute_named\n(from pkg import a,b)"]
    Q --> R["importlib.import_module(abs_pkg)\ngetattr → mod.__dict__[name]\n__all__.append(name)"]

    R --> S["Resolve relative_wildcards\n(from .X import *)"]
    S --> T["importlib.import_module(fq_name)\nfor each public name → mod.__dict__"]

    T --> U{fallback_packages?}
    U -- Yes --> V["Build lazy _pkg_getattr\n__getattr__ chains stub → pkg fallback"]
    U -- No --> W["__getattr__ = stub_getattr"]

    V --> X["setattr mod: __getattr__, __dir__, __all__"]
    W --> X
Loading

Last reviewed commit: "Add absolute named i..."

Comment on lines +68 to +69
names = [alias.name for alias in node.names]
absolute_named.setdefault(node.module, []).extend(names)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Import aliases silently dropped

alias.asname is never consulted, so from pkg import foo as bar in a stub will eagerly register the attribute under the original name foo (not the alias bar). The correct exported key should be alias.asname if alias.asname else alias.name, and the value should always be fetched as alias.name from the source module.

In practice, stubs in this project may not currently use as-aliasing for absolute named imports, but the silent misbehaviour is a latent bug: the wrong name ends up in mod.__dict__ and __all__, and the intended alias is never exported.

Suggested change
names = [alias.name for alias in node.names]
absolute_named.setdefault(node.module, []).extend(names)
names = [(alias.name, alias.asname or alias.name) for alias in node.names]
for src_name, exported_name in names:
absolute_named.setdefault(node.module, []).append((src_name, exported_name))

And correspondingly in lazy_export:

for abs_pkg, name_pairs in absolute_named.items():
    pkg_mod = importlib.import_module(abs_pkg)
    for src_name, exported_name in name_pairs:
        mod.__dict__[exported_name] = getattr(pkg_mod, src_name)
        if exported_name not in __all__:
            __all__.append(exported_name)

Comment on lines +155 to +160
for abs_pkg, names in absolute_named.items():
pkg_mod = importlib.import_module(abs_pkg)
for name in names:
mod.__dict__[name] = getattr(pkg_mod, name)
if name not in __all__:
__all__.append(name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No error context on failed absolute named import

Both importlib.import_module(abs_pkg) and getattr(pkg_mod, name) can raise without any pointer back to the stub that declared the import. Compare with the analogous fallback_packages block (lines 176–184) which wraps import_module in a try/except and re-raises an ImportError with a descriptive message.

Consider wrapping this block similarly:

for abs_pkg, names in absolute_named.items():
    try:
        pkg_mod = importlib.import_module(abs_pkg)
    except ImportError as e:
        raise ImportError(
            f"lazy_export() in {package_name!r}: .pyi stub declares "
            f"'from {abs_pkg} import ...' but the package is not installed."
        ) from e
    for name in names:
        try:
            mod.__dict__[name] = getattr(pkg_mod, name)
        except AttributeError as e:
            raise AttributeError(
                f"lazy_export() in {package_name!r}: .pyi stub declares "
                f"'from {abs_pkg} import {name}' but the attribute does not exist."
            ) from e
        if name not in __all__:
            __all__.append(name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant