pytorch / pytorch/pytorch

torch.compile: isinstance() with a runtime_checkable Protocol constant-folds the result without guarding the attributes it reads

Open
#196,127 1 comment 0 reactions 0 assignees View on GitHub
bot-triaged high priority module: correctness (silent) module: dynamo module: guards oncall: pt2 release triage triaged
Dominant language
Python
Stars
103k
Forks
29.5k
PR merge metrics
PR metrics pending

Description

Found while working on #195969. The union/tuple bug reported there is a separate
issue; this one is about the single-class path that already works "correctly" on
main, and it predates that report. Analysis and reproducer below were produced
with an AI assistant and are quoted as such; I verified the reproducer myself on
current main.

> ### Summary
>
> `isinstance(obj, SomeProtocol)` is constant-folded into the graph at trace time,
> but no guard is installed on the attributes the Protocol check actually reads.
> A `@runtime_checkable` Protocol with non-method (data) members inspects the
> **instance**, while the cache entry only discriminates on the **type**. Two
> instances of the same class that differ in whether the attribute is set share a
> cache entry, so the second one silently gets the first one's answer.
>
> ### Reproducer
>
> ```python
> from typing import Protocol, runtime_checkable
>
> import torch
>
>
> @runtime_checkable
> class HasPorts(Protocol):
> ports: tuple[int, ...]
>
>
> class Obj:
> def __init__(self, with_ports):
> if with_ports:
> self.ports = (1, 2)
>
>
> def fn(x, o):
> return x + 1 if isinstance(o, HasPorts) else x - 1
>
>
> x = torch.ones(3)
> opt_fn = torch.compile(fn, backend="eager")
>
> has, lacks = Obj(True), Obj(False)
> print("with ports: eager", fn(x, has)[0].item(), " compiled", opt_fn(x, has)[0].item())
> print("without ports: eager", fn(x, lacks)[0].item(), " compiled", opt_fn(x, lacks)[0].item())
> print("recompiled:", torch._dynamo.utils.counters["stats"]["unique_graphs"] > 1)
> ```
>
> Output:
>
> ```text
> with ports: eager 2.0 compiled 2.0
> without ports: eager 0.0 compiled 2.0
> recompiled: False
> ```
>
> The second call is wrong and there is no recompilation, no graph break and no
> warning. Swapping the call order gives the mirrored wrong answer, which confirms
> that whichever instance is seen first wins.
>
> ### Cause
>
> `BuiltinVariable.call_isinstance` in `torch/_dynamo/variables/builtin.py`
> evaluates the metaclass hook on the real object and builds a constant from the
> result:
>
> ```python
> if (
> isinstance(arg, variables.UserDefinedObjectVariable)
> and "__instancecheck__" in isinstance_type.__class__.__dict__
> ):
> return VariableTracker.build(
> tx,
> isinstance_type.__class__.__instancecheck__(isinstance_type, arg.value),
> )
> ```
>
> Running the hook is what makes Protocols work at all, so the call itself is not
> the problem. The problem is that the result is folded into the graph without
> recording what it depended on. For a data-member Protocol,
> `_ProtocolMeta.__instancecheck__` performs a `hasattr()` per member, and none of
> those lookups turn into guards.
>
> The same shape of hole applies to any metaclass `__instancecheck__` that reads
> instance state rather than the type, so a hypothetical fix should probably not
> be Protocol-specific.
>
> ### Possible directions
>
> 1. Install attribute-presence guards for the members a `runtime_checkable`
> Protocol inspects (`__protocol_attrs__`). Narrow and cheap, but only covers
> Protocols and reaches into `typing` internals.
> 2. Graph break instead of constant-folding when the classinfo has a hook that is
> not on a known-safe allowlist. Sound, but would regress code that relies on
> the current behavior, which is a significant amount given how long it has
> worked this way.
> 3. Keep folding, but treat the hook result as a value that needs its own guard
> mechanism.
>
> I do not have a strong opinion on which is right, hence this issue rather than a
> PR.
>
> ### Versions
>
> Reproduced on current main (2.15.0a0) and on the released 2.14.0, CPython 3.14,
> Windows. `backend="eager"` is used to show it is Dynamo-level; inductor behaves
> the same.

cc @ezyang @gchanan @kadeng @msaroufim @chauhang @penguinwu @voznesenskym @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @amjames @jataylo @azahed98 @anijain2305 @williamwen42 @jansel

Contributor guide

Open the contributing guide

Research direction

Start with the reproducer in the issue, then read BuiltinVariable.call_isinstance in torch/_dynamo/variables/builtin.py and trace how the Protocol __instancecheck__ reads instance attributes. Compare compiled and eager results for both call orders; done means differing instances produce correct results without silently sharing an unguarded constant-folded answer.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, pytorch
Domain
compilers, machine-learning
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.