Fix lazy magic lookup resolving the wrong provider per kind by bunnysayzz · Pull Request #15387 · ipython/ipython

bunnysayzz · 2026-09-08T11:39:41Z

When one magic name is lazily declared with a different provider per kind, load_lazy() always preferred the line-table placeholder, so %%name? loaded the wrong provider and left a stale cell placeholder behind. On retry the stale placeholder was dropped, which made the outcome depend on registration order: line-then-cell worked on the second attempt, cell-then-line never worked at all.

This passes the requested kind from find() into load_lazy(), so each kind loads its own provider on the first lookup. It also remembers a displaced lazy declaration per (kind, name): if the newer declaration never delivers the magic, the previous one is restored for one retry instead of the name going unresolvable. That last part covers the time case in the issue, where a line-only override kept working for %time/time? while %%time? and %%time fell back to IPython's own cell magic again.

Tests added in tests/test_magic.py (test_lazy_magic_help_finds_both_kinds_first_try, parametrized over both registration orders, and test_lazy_magic_falls_back_to_previous_declaration). All three fail without the fix and pass with it. Full test_magic.py + test_magic_table.py green (149 passed, 5 skipped); the 5 failures in test_interactiveshell.py/test_prefilter.py are pre-existing environment failures, identical on clean main.

@Darshan808 would appreciate your review when you have a moment.

Darshan808

Thanks @bunnysayzz for working on this. Left few comments.

Darshan808 · 2026-09-09T10:25:50Z

+                # A newer declaration displaces an older one for this kind;
+                # remember the old spec so it can be restored if the new
+                # declaration never delivers the magic.
+                self._lazy_fallbacks[(kind, name)] = existing.spec


One thing though: _lazy_fallbacks only remembers one level, so it breaks as soon as a name gets declared twice:

@magics_class class MyMagics(Magics): @line_magic def time(self, line): """My own %time.""" @magics_class class MyAnotherMagics(Magics): @line_magic def time(self, line): """MyAnother own %time.""" sys.modules["provider"] = provider = types.ModuleType("provider") provider.MyMagics, provider.MyAnotherMagics = MyMagics, MyAnotherMagics ip.magics_manager.register_lazy("time", "provider:MyMagics") ip.magics_manager.register_lazy("time", "provider:MyAnotherMagics") %time? # MyAnother own %time. ✅ %%time? # nothing — built-in %%time is gone ❌

The second register_lazy overwrites _lazy_fallbacks[("cell", "time")] with provider:MyMagics, so the built-in's spec is lost. find then restores MyMagics, which is also line-only, doesn't deliver a cell magic either, and hits the del, so %%time is destroyed for the session. Same symptom as the original case C in the issue, just one declaration later.

Darshan808 · 2026-09-09T10:26:13Z

        # wholesale, so prefer the spec the placeholder carries.
-        fn = self.magics["line"].get(magic_name) or self.magics["cell"].get(magic_name)
+        if magic_kind is not None:
+            fn = self.magics[magic_kind].get(magic_name)


Good catch here!

Darshan808 · 2026-09-09T10:33:34Z

+                # Declared but not delivered. If an older declaration for
+                # this kind was displaced, restore it and give it one
+                # chance rather than dropping the name entirely.
+                fallback = self._lazy_fallbacks.pop((magic_kind, magic_name), None)
+                if fallback is not None and fallback != fn.spec:
+                    self.magics[magic_kind][magic_name] = LazyMagic(
+                        self, fallback, magic_kind, magic_name
+                    )
+                    self.load_lazy(magic_name, magic_kind)
+                    fn = self.magics[magic_kind].get(magic_name)
+                if isinstance(fn, LazyMagic):
+                    # Still nothing delivered; drop the stale placeholder.
+                    del self.magics[magic_kind][magic_name]
+                    fn = None


Well, storing the displaced entry on the LazyMagic that displaced it instead of in a side table gives you arbitrary depth for free, and it can't get out of sync with the magics table.

Consider this:

# LazyMagic.__init__ self.shadowed = shadowed # register_lazy self.magics[kind][name] = LazyMagic(self, fully_qualified_name, kind, name, shadowed=existing) # find — instead of the single retry + del while isinstance(fn, LazyMagic): self.load_lazy(magic_name, magic_kind) if table.get(magic_name) is not fn: fn = table.get(magic_name) continue fn = fn.shadowed if fn is None: del table[magic_name] else: table[magic_name] = fn

bunnysayzz · 2026-09-09T10:51:14Z

Thanks, both points taken. Reworked per your sketch: the displaced entry now rides on the placeholder itself (shadowed), and find swaps each undispatched placeholder into the table and retries, so the chain unwinds to whatever declaration actually delivers, at any depth. Side table is gone.

Added test_lazy_magic_falls_back_through_two_declarations (two newest declarations deliver nothing, oldest resolves) — I verified it fails on the old single-level code and passes now. Full test_magic.py + test_magic_table.py still 150 passed, and the reformatted lines match the formatter.

bunnysayzz · 2026-09-09T11:48:15Z

Good catch, same root cause one layer over. register_lazy overwrites lazy_magics[name] per call, and load_all then fed that single spec through kind-blind load_lazy, so only the line half's class ever imported. Now load_all_lazy_magics walks the placeholders per kind first (extensions still skipped, same as before) and keeps the old loop as fallback. Added test_load_all_lazy_magics_loads_both_kind_halves; verified it plus the other four new tests fail on clean main and pass here. Suite still 151 green.

Darshan808 · 2026-09-09T12:34:19Z

+            line_spec = f"{mod}:LineHalf"
+            cell_spec = f"{mod}:CellHalf"
+            try:
+                mm.register_lazy("dual", line_spec, "line")  # type: ignore[arg-type]


Suggested change

mm.register_lazy("dual", line_spec, "line") # type: ignore[arg-type]

mm.register_lazy("dual", line_spec, "line")

Darshan808 · 2026-09-09T12:34:54Z

+                else (("cell", cell_spec), ("line", line_spec))
+            )
+            try:
+                mm.register_lazy("dual", first[1], first[0])  # type: ignore[arg-type]


Suggested change

mm.register_lazy("dual", first[1], first[0]) # type: ignore[arg-type]

mm.register_lazy("dual", first[1], first[0])

Darshan808 · 2026-09-09T12:35:13Z

+            )
+            try:
+                mm.register_lazy("dual", first[1], first[0])  # type: ignore[arg-type]
+                mm.register_lazy("dual", second[1], second[0])  # type: ignore[arg-type]


Suggested change

mm.register_lazy("dual", second[1], second[0]) # type: ignore[arg-type]

mm.register_lazy("dual", second[1], second[0])

Darshan808 · 2026-09-09T12:35:32Z

+            cell_spec = f"{mod}:CellHalf"
+            try:
+                mm.register_lazy("dual", line_spec, "line")  # type: ignore[arg-type]
+                mm.register_lazy("dual", cell_spec, "cell")  # type: ignore[arg-type]


Suggested change

mm.register_lazy("dual", cell_spec, "cell") # type: ignore[arg-type]

mm.register_lazy("dual", cell_spec, "cell")

Darshan808 · 2026-09-09T15:36:50Z

+            try:
+                mm.register_lazy("dual", line_spec, "line")  # type: ignore[arg-type]
+                mm.register_lazy("dual", cell_spec, "cell")  # type: ignore[arg-type]
+                mm.load_all_lazy_magics()


Calling load_all_lazy_magics() on the shared session shell may break later tests.
Lets build a standalone MagicsManager(shell=...) for this test instead of using the session ip. There's a manager fixture in test_magic_table.py

Darshan808 · 2026-09-09T15:49:54Z

+        # Load per kind from the placeholders themselves: one name may be
+        # declared with a different provider per kind, and ``lazy_magics``
+        # only remembers the last spec per name. Extensions (specs without a
+        # ":") are still skipped: importing one can run arbitrary code.
+        for kind in magic_kinds:
+            for magic_name in list(self.magics[kind]):
+                fn = self.magics[kind].get(magic_name)
+                if isinstance(fn, LazyMagic) and ":" in fn.spec:
+                    self.load_lazy(magic_name, kind)


One suggestion: the two loops here are deriving "everything still declared lazily" from the two stores inline, and _MagicsRegistry.__missing__ needs the same thing. Worth pulling it into a helper?

def _lazy_declarations(self) -> list[tuple[str, _MagicKind | None, str]]: """Every live lazy declaration, as ``(name, kind, spec)``. The placeholders in :attr:`magics` are the source of truth, because :attr:`lazy_magics` is keyed by name alone and so keeps only the last spec for a name declared once per kind. A name declared only through the trait has no placeholder, and so no known kind. """ seen: set[tuple[str, str]] = set() declarations: list[tuple[str, _MagicKind | None, str]] = [] for kind in magic_kinds: for name, fn in list(self.magics[kind].items()): if isinstance(fn, LazyMagic) and (name, fn.spec) not in seen: seen.add((name, fn.spec)) declarations.append((name, kind, fn.spec)) for name, spec in list(self.lazy_magics.items()): if (name, spec) not in seen: seen.add((name, spec)) declarations.append((name, None, spec)) return declarations

Then this method is just a filter:

def load_all_lazy_magics(self) -> None: for magic_name, magic_kind, spec in self._lazy_declarations(): if ":" in spec: self.load_lazy(magic_name, magic_kind)

__missing__ with the helper:

def __missing__(self, key: s for magic_name, magic_kind, spec in self._manager._lazy_declarations(): if spec.endswith(":" self._manager.load_lazy(magic_name, magic_kind) # A matching spelared once per kind # has one spec per kind, so keep going until `key` shows up. if key in self: break if key not in self: raise KeyError(key) return self[key]

bunnysayzz · 2026-09-09T16:26:00Z

Round 2 done, thanks for the detailed review:

Helper: added MagicsManager._lazy_declarations() essentially as you sketched it, and load_all_lazy_magics is now just the filter over it. _MagicsRegistry.__missing__ uses it too: it keeps loading matching specs until key shows up instead of stopping after the first trait hit.
Standalone manager: the load_all test now builds its own MagicsManager(shell=...) via a local fixture (same shape as the one in test_magic_table.py), so the shared session shell is untouched and the try/finally cleanup shrank to just sys.modules.
Arg-order suggestions: I double-checked these against the signature (name, fully_qualified_name, magic_kind) and the calls already pass (dual, spec, kind) — the tuples are (line, line_spec) etc., indexed [1], [0]. E.g. line 2023 unpacks to (dual, line_spec, line), which is what the suggestion shows. So I left those four lines as-is; if you were seeing a different version, let me know.

All green locally: test_magic.py + test_magic_table.py, 153 passed, 3 skipped. I also re-verified the load_all test still fails if the helper is neutered back to trait-only iteration, so it still guards the original bug.

Carreau · 2026-09-11T08:36:05Z

+        # The table entry this declaration displaced, if any. A name may be
+        # declared lazily any number of times; the chain is walked if a
+        # newer declaration never delivers the magic.
+        self.shadowed = shadowed


You likely want to create docstring for __init__ and start documenting the parameter instead of comments.

And that seem like overkill, if someone called register_magic with the wrong kind, and it does not "deliver" (A lot of that vocabulary seem a lot like opus 5) then I think it is fair to error or do the wrong things than to try to walk to the next declaration (unless I missunderstand)

I think this solution is for a particular issue that @krassowski described in the issue thread.

@magics_class class MyMagics(Magics): @line_magic def time(self, line): """My own %time.""" sys.modules["provider"] = provider = types.ModuleType("provider") provider.MyMagics = MyMagics ip = InteractiveShell() ip.magics_manager.register_lazy("time", "provider:MyMagics") ip.run_cell("time?") # my own %time ip.run_cell("%%time?") # expected: IPython's %%time docs ip.run_cell("%%time\npass") # expected: a wall time report

if someone called register_magic with the wrong kind, and it does not "deliver"

Yes a direct solution would be to just use:

ip.magics_manager.register_lazy("time", "provider:MyMagics", "line")

but by default it is "line_cell" and overrides both %time and %%time without checking if it provides both implementation (and I don't think we can validate since the magic is registered lazily and the provider module has not been imported yet), which is causing the above failure.

I think it is fair to error or do the wrong things than to try to walk to the next declaration

I also agree with this, since it is a mistake on the side of the magic registration too. Maybe we could look into improving the error message. I'll see.

Thanks, that matches my read too. The walk only matters for the line_cell-default displacement case in your example, and I agree the registration itself is the real mistake. I am fine either way: keep the walk as a safety net, or drop it and surface a clearer error when a lazily registered name resolves to a provider that does not implement the requested kind. Say which you prefer and I will do it. No code changes in the meantime, the branch is green as is.

bunnysayzz · 2026-09-11T10:38:34Z

Thanks for looking, both points taken seriously:

Docstring over inline comment: agreed, I'll move the shadowed explanation into an __init__ docstring documenting the parameter. (Also noted on the vocabulary, I'll use plainer words.)

Is the walk overkill? Let me make sure the case it covers is worth it before I rip it out. The concrete scenario is the time test: IPython itself declares time lazily for both kinds. A user then registers their own line-only time, which displaces both placeholders. When something looks up %%time, the new provider loads fine but delivers no cell magic. Without the walk, the cell lookup ends with the name resolving to nothing, so %%time breaks entirely as a side effect of overriding %time. The walk restores the previously displaced declaration in that case, so %%time keeps working.

If you'd rather that cell lookup raise (or just resolve to whatever the new provider gave) instead of falling back, say the word and I'll simplify it down to that. Your call.

bunnysayzz · 2026-09-11T18:22:53Z

I applied the documentation-only part of your first note in commit fda3b63f1: the LazyMagic.__init__ docstring now documents shadowed, and the old explanatory inline comment is gone. I left the fallback behavior unchanged while waiting for your answer on whether the displaced-entry walk should be removed. The magic test suites remain green: 153 passed, 3 skipped.

Darshan808

This mostly looks good now. The only thing to discuss about is what we should do when the magic class doesn't provide an implementation it's expected to have. Should we raise an error, or walk back to the previous declaration like this PR proposes? Any thoughts, @Carreau and @krassowski?

krassowski · 2026-09-23T14:24:06Z

It is probably easier to raise an error and then permit fallback than shipping it the other way round?

Darshan808 · 2026-09-23T17:28:05Z

Makes sense!
@bunnysayzz, would you be able to remove the fallback logic for now and just raise an error if the magic class is missing an implementation it's expected to provide?

…isplaced declarations When one name is lazily declared with a different provider per kind, load_lazy() always preferred the line placeholder, so %%name? loaded the wrong provider, left a stale cell placeholder, and failed. On retry the stale placeholder was dropped, which made the outcome depend on registration order: line-then-cell worked on second attempt, cell-then-line never worked. Pass the requested kind from find() into load_lazy() so each kind loads its own provider on first lookup. Also, when a newer declaration displaces an older one for a kind but never delivers it, restore the previous declaration for one retry instead of dropping the name, so e.g. a line-only override of time keeps the builtin cell time working. Fixes ipython#15383

… test

…ions Per the review direction (krassowski + Darshan808): when a lazily declared class does not provide an implementation for the requested kind, find() raises UsageError naming the magic and spec instead of walking back to an older declaration. The shadow chain (LazyMagic shadowed link, register_lazy displacement recording) is removed; _declarations stays for __missing__/load_all. The two fallback tests plus the placeholder-drop test now assert the raise, each proven to fail pre-fix.

bunnysayzz · 2026-09-23T18:55:11Z

Done, fallback is out. find() now raises UsageError naming the magic and spec when a declared class does not deliver the kind, and the whole shadow chain (LazyMagic link, register_lazy recording) is gone. The two fallback tests plus the placeholder-drop test now assert the raise, each proven to fail pre-fix. Full test_magic + test_magic_table green (152 passed, 4 skipped, the skips pre-existing).

Darshan808 · 2026-09-24T17:03:41Z

        """
        fn = self.magics[magic_kind].get(magic_name)
        if isinstance(fn, LazyMagic) or (fn is None and magic_name in self.lazy_magics):
-            self.load_lazy(magic_name)
-            fn = self.magics[magic_kind].get(magic_name)
-            if isinstance(fn, LazyMagic):
-                # Declared but not delivered; drop the stale placeholder.
-                del self.magics[magic_kind][magic_name]
-                fn = None
+            while isinstance(fn, LazyMagic):
+                self.load_lazy(magic_name, magic_kind)
+                current = self.magics[magic_kind].get(magic_name)
+                if current is not fn:
+                    # The load delivered something (or cleared the name).
+                    fn = current
+                    continue


Suggested change

"""

fn = self.magics[magic_kind].get(magic_name)

if isinstance(fn, LazyMagic) or (fn is None and magic_name in self.lazy_magics):

self.load_lazy(magic_name)

fn = self.magics[magic_kind].get(magic_name)

if isinstance(fn, LazyMagic):

# Declared but not delivered; drop the stale placeholder.

del self.magics[magic_kind][magic_name]

fn = None

while isinstance(fn, LazyMagic):

self.load_lazy(magic_name, magic_kind)

current = self.magics[magic_kind].get(magic_name)

if current is not fn:

# The load delivered something (or cleared the name).

fn = current

continue

Raises

------

UsageError

If `magic_name` is declared lazily for `magic_kind` but what the

declaration names does not provide it.

"""

table = self.magics[magic_kind]

fn = table.get(magic_name)

if fn is None and magic_name in self.lazy_magics:

# Declared straight into `lazy_magics` as configuration, so there

# is no placeholder for this kind to carry the spec.

self.load_lazy(magic_name, magic_kind)

fn = table.get(magic_name)

if isinstance(fn, LazyMagic):

self.load_lazy(magic_name, magic_kind)

fn = table.get(magic_name)

if isinstance(fn, LazyMagic):

Darshan808 · 2026-09-24T17:06:40Z

@bunnysayzz Could you integrate the above change request and fix the formatting too.

Darshan808's round-3 inline suggestion: numpydoc Raises section on find(), table local, trait-declared vs placeholder paths split out. Also collapsed the LazyMagic construction to the black-clean single line (remaining black diffs in the file are pre-existing version drift, verified on the clean tree).

bunnysayzz · 2026-09-24T18:56:13Z

Done, applied the suggestion as written (Raises section, table local, trait vs placeholder split) and collapsed the LazyMagic call to the black-clean line. Full test_magic + test_magic_table still green. Note: this black version flags a few hunks elsewhere in the file, but those fail identically on the clean tree, so I left them alone.

Darshan808

This looks good to me.

bunnysayzz · 2026-09-27T22:31:23Z

the one red check is the downstream pytest ipykernel step, a collection error inside ipykernels own suite (their timeout marker config), exit code 4. all the ipython suites are green, and this fails without my change too, so its unrelated to the PR. i cant re-run upstream jobs from a fork, if that check blocks merging would you mind kicking it?

bunnysayzz mentioned this pull request Sep 8, 2026

Custom lazy magics do not show help (?) until they are imported #15383

Open

Darshan808 suggested changes Sep 9, 2026

View reviewed changes

Darshan808 added the bug label Sep 9, 2026

Darshan808 suggested changes Sep 9, 2026

View reviewed changes

Carreau reviewed Sep 11, 2026

View reviewed changes

krassowski requested a review from Darshan808 September 19, 2026 10:11

krassowski added this to the 9.18 milestone Sep 19, 2026

Darshan808 reviewed Sep 19, 2026

View reviewed changes

bunnysayzz added 7 commits September 24, 2026 00:24

Format the displaced-declaration guard per darker

1725aa8

Rework displaced-lazy fallback as a shadow chain on the placeholder

1d3ca1f

Load every kind's lazy provider in load_all_lazy_magics

b34ea9b

Address review round 2: _lazy_declarations helper, standalone manager…

833a5be

… test

docs(magic): document displaced lazy entries

3164973

bunnysayzz force-pushed the fix/lazy-magic-help-both-kinds-15383 branch from fda3b63 to d434b66 Compare September 23, 2026 18:54

Darshan808 reviewed Sep 24, 2026

View reviewed changes

Darshan808 suggested changes Sep 24, 2026

View reviewed changes

Darshan808 approved these changes Sep 27, 2026

View reviewed changes

	mm.register_lazy("dual", line_spec, "line") # type: ignore[arg-type]
	mm.register_lazy("dual", line_spec, "line")

	mm.register_lazy("dual", first[1], first[0]) # type: ignore[arg-type]
	mm.register_lazy("dual", first[1], first[0])

	mm.register_lazy("dual", second[1], second[0]) # type: ignore[arg-type]
	mm.register_lazy("dual", second[1], second[0])

	mm.register_lazy("dual", cell_spec, "cell") # type: ignore[arg-type]
	mm.register_lazy("dual", cell_spec, "cell")

Uh oh!

Conversation

bunnysayzz commented Sep 8, 2026

Uh oh!

Darshan808 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 9, 2026

Uh oh!

bunnysayzz commented Sep 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 11, 2026

Uh oh!

bunnysayzz commented Sep 11, 2026

Uh oh!

Darshan808 left a comment

Choose a reason for hiding this comment

Uh oh!

krassowski commented Sep 23, 2026

Uh oh!

Darshan808 commented Sep 23, 2026

Uh oh!

bunnysayzz commented Sep 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Darshan808 commented Sep 24, 2026

Uh oh!

bunnysayzz commented Sep 24, 2026

Uh oh!

Darshan808 left a comment

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 27, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants