Fix lazy magic lookup resolving the wrong provider per kind - #15387
bunnysayzz wants to merge 6 commits into
Conversation
…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
Darshan808
left a comment
Darshan808
left a comment
There was a problem hiding this comment.
Thanks @bunnysayzz for working on this. Left few comments.
There was a problem hiding this comment.
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.
| # 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) |
| # 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 |
There was a problem hiding this comment.
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
|
Thanks, both points taken. Reworked per your sketch: the displaced entry now rides on the placeholder itself ( Added |
|
Good catch, same root cause one layer over. |
| line_spec = f"{mod}:LineHalf" | ||
| cell_spec = f"{mod}:CellHalf" | ||
| try: | ||
| mm.register_lazy("dual", line_spec, "line") # type: ignore[arg-type] |
There was a problem hiding this comment.
| mm.register_lazy("dual", line_spec, "line") # type: ignore[arg-type] | |
| mm.register_lazy("dual", line_spec, "line") |
| else (("cell", cell_spec), ("line", line_spec)) | ||
| ) | ||
| try: | ||
| mm.register_lazy("dual", first[1], first[0]) # type: ignore[arg-type] |
There was a problem hiding this comment.
| mm.register_lazy("dual", first[1], first[0]) # type: ignore[arg-type] | |
| mm.register_lazy("dual", first[1], first[0]) |
| ) | ||
| 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] |
There was a problem hiding this comment.
| mm.register_lazy("dual", second[1], second[0]) # type: ignore[arg-type] | |
| mm.register_lazy("dual", second[1], second[0]) |
| 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] |
There was a problem hiding this comment.
| mm.register_lazy("dual", cell_spec, "cell") # type: ignore[arg-type] | |
| mm.register_lazy("dual", cell_spec, "cell") |
| 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() |
There was a problem hiding this comment.
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
| # 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) |
There was a problem hiding this comment.
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 declarationsThen 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]
|
Round 2 done, thanks for the detailed review:
All green locally: |
| # 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 |
There was a problem hiding this comment.
You likely want to create docstring for __init__ and start documenting the parameter instead of comments.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 reportif 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.
There was a problem hiding this comment.
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.
|
Thanks for looking, both points taken seriously: Docstring over inline comment: agreed, I'll move the 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 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. |
3 participants
Fixes #15383.
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()intoload_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 thetimecase in the issue, where a line-only override kept working for%time/time?while%%time?and%%timefell 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, andtest_lazy_magic_falls_back_to_previous_declaration). All three fail without the fix and pass with it. Fulltest_magic.py+test_magic_table.pygreen (149 passed, 5 skipped); the 5 failures intest_interactiveshell.py/test_prefilter.pyare pre-existing environment failures, identical on clean main.@Darshan808 would appreciate your review when you have a moment.