benchmark
cd3213b7 - Split DictKeySetVariable off SetVariable to match CPython (#193649)

Commit
57 days ago
Split DictKeySetVariable off SetVariable to match CPython (#193649) Summary: ## Issue Part of #192874, the `DictKeySetVariable(SetVariable)` row. That tracker lists eight rows, so this does not close it. ## Summary `DictKeySetVariable` traces a `dict_keys` view that is passed *into* the compiled region (`builder.py`, the `isinstance(value, dict_keys)` branch). Tracing `d.keys()` inside the region produces `DictKeysVariable` instead, which is a different class and already behaves correctly; this PR does not touch it. Because `DictKeySetVariable` subclassed `SetVariable`, a traced `dict_keys` picked up all 16 of `set`'s named methods through the `tp_methods` MRO merge. Under `fullgraph=True` six of them silently returned a value for code that raises in eager: ```python import torch def f(k): return k.union({3}) # dict_keys has no .union torch.compile(f, backend="eager", fullgraph=True)({1: 0, 2: 0}.keys()) ``` Before: `{1, 2, 3}`. After: an ordinary unsupported-call error, and eager still raises `AttributeError: 'dict_keys' object has no attribute 'union'`. The same applied to `intersection`, `difference`, `symmetric_difference`, `copy` and `discard`. Eight more leaked an internal error instead of `AttributeError`: `add`, `clear`, `intersection_update`, `difference_update` and `symmetric_difference_update` hit `AssertionError: mutation_type is None for DictKeySetVariable() in check_allowed_side_effect`, while `issubset`, `issuperset` and `pop` raised `InternalTorchDynamoError`. The inherited slots were wrong in the other direction too, and this part breaks code that works in eager: - `k | s`, `k & s`, `k - s`, `k ^ s` raised `Unsupported: Observed exception` under `fullgraph=True`. `SetVariable`'s number slots gate on `pyanyset_check`, which is `issubclass(python_type(), (set, frozenset))`, and `dict_keys` is neither, so every slot returned `NotImplemented`. - `k == s`, `k <= s`, `k >= s` raised `InternalTorchDynamoError: TypeError: '<=' not supported between instances of 'dict' and 'set'` in **both** graph-break and fullgraph modes. `tp_richcompare_impl` compares `self.set_items` against `other.set_items`, but `DictKeySetVariable.set_items` returns the internal `dict` where every other set VT returns a `set`. ## Fix `dict_keys` is a `collections.abc.Set` but not a `set`. That is the whole shape of the fix, and CPython states it precisely: ```python >>> sorted(d for d in dir(collections.abc.Set) if not d.startswith("_")) ['isdisjoint'] >>> sorted(d for d in dir({}.keys()) if not d.startswith("_")) ['isdisjoint', 'mapping'] >>> issubclass(collections.abc.KeysView, collections.abc.Set) True ``` `DictKeySetVariable` now derives from `BaseSetVariable`, the shared base #193580 introduced for `frozenset`, instead of `SetVariable`, so it can reach none of `set`'s mutators or `nb_inplace_*` slots. `BaseSetVariable.tp_methods` also had to shrink. `tp_methods` merges additively along the MRO (a subclass table can add entries but never drop one), and the base registered eight read-only methods, seven of which `dict_keys` does not have. So `isdisjoint` is the only entry left on `BaseSetVariable`, matching the `Set` ABC, and the other seven (`union`, `intersection`, `difference`, `symmetric_difference`, `issubset`, `issuperset`, `copy`) stay *implemented* on `BaseSetVariable` but are *registered* by `SetVariable.tp_methods` and `FrozensetVariable.tp_methods`, as `Method(BaseSetVariable.union)` and so on, the form `lists.py` already uses. That is also how CPython lays it out: `set_methods` and `frozenset_methods` are separate tables sharing one C implementation. `DictKeySetVariable` then gets the dict-view slots it should have had: `nb_or_impl` / `nb_and_impl` / `nb_subtract_impl` / `nb_xor_impl` mirroring `dictviews_or` and friends, which build a set from the left operand and update it with the right, so either side may be any iterable and the result is always a plain `set`: ```python >>> {1: 0, 2: 0}.keys() | [3] # a list operand is fine, unlike set.__or__ {1, 2, 3} >>> [1, 3] - {1: 0, 2: 0}.keys() # and it is symmetric {3} ``` plus a `tp_richcompare_impl` mirroring `dictview_richcompare`, which accepts any set or dict view and returns `NotImplemented` otherwise. The shared comparison body moved to `BaseSetVariable._richcompare_items`; set's operand check stays narrow, matching CPython, where `set_richcompare` uses `PyAnySet_Check` and lets the reflected dict-view op handle `set == dict_keys`. Three things `DictKeySetVariable` used to inherit from `SetVariable` and `BaseSetVariable` does not provide are now defined on it: `is_hashable` and `hash_impl` (`dict_keys` is unhashable), and `tp_repr_impl`, which now produces `dict_keys([1, 2])` where it used to produce `{1, 2}`. Eight `isinstance` checks outside `sets.py` that enumerate `(SetVariable, FrozensetVariable)` gain `DictKeySetVariable` (`builtin.py` x3, `higher_order_ops.py` x2, `lists.py`, `object_protocol.py`, and `_unpack_fast_types()` in `utils.py`), following the enumeration style #193580 landed with. Four `isinstance(..., SetVariable)` checks are deliberately left narrow, because the split makes them more accurate: `SET_ADD` and `SET_UPDATE` in `symbolic_convert.py` assert their accumulator is a mutable set, and the `const_dict_or_set_mutation` replay handler in `side_effects.py` emits `update` and `clear`. A `dict_keys` is never a comprehension accumulator and is never mutated. ## Test plan New `DictKeySetHierarchyTests` in `test/dynamo/test_sets.py`, 10 tests. Same test file, only `torch/_dynamo` swapped (unpatched is today's nightly, which already has `BaseSetVariable` from #193580): ``` python test/dynamo/test_sets.py -k DictKeySetHierarchyTests unpatched Ran 10 tests, FAILED (failures=17, errors=22) patched Ran 10 tests, OK ``` Being precise about what that shows, since not all ten are regression coverage. Five pass both before and after and are non-regression guards: `test_set_abc_surface_still_works`, `test_binary_op_with_non_iterable_raises`, `test_ordering_against_non_set_raises`, `test_regular_set_behaviour_unaffected` and `test_dict_view_operands_match_eager`. That last one guards a trap this change can fall into: a view built *inside* the region is a `DictKeysVariable`, which this VT cannot read directly, so comparing against one has to fall through to the reflected operation. `test_dict_keys_is_not_a_set_variable` is structural, guarding against the classes being merged again. The existing `FrozensetHierarchyTests.test_frozenset_readonly_ops_still_work` covers the `tp_methods` relocation for `frozenset` (`union`, `issubset`, `issuperset`, `copy` all still trace). Suites run locally, all against the patched tree: ``` python test/dynamo/test_sets.py Ran 206 tests, OK (skipped=1) python test/dynamo/test_dicts.py Ran 325 tests, OK (expected failures=1) python test/dynamo/test_functions.py Ran 565 tests, OK (skipped=9, expected failures=2) python test/dynamo/test_higher_order_ops.py Ran 225 tests, OK (skipped=17, expected failures=1) python test/dynamo/test_repros.py Ran 389 tests, OK (skipped=19, expected failures=4) python test/dynamo/test_misc.py Ran 860 tests, OK (skipped=14, expected failures=4) once ninja is on PATH; without it test_pybind11_enum_conversion errors with "Ninja is required to load C++ extensions" ``` Lint: ``` ruff 0.16.3 All checks passed! / 7 files already formatted pyrefly 0.58.0 131 errors, all pre-existing missing-attribute reports against torch._C (build-generated stubs absent in a non-source checkout); 0 on lines this change adds, 0 in sets.py ``` These ran against the nightly CPU wheel `2.15.0.dev20260901+cpu` with the repository `torch/_dynamo` overlaid, rather than a from-source build. The change is pure Python with no C++ coupling, so the modified code is the code under test, but no CUDA or distributed suites were run locally. ## BC-breaking? No for user-visible behaviour: code that worked in eager keeps working, and several cases that silently returned wrong values or leaked internal errors now match eager. For out-of-tree code touching Dynamo internals, `isinstance(x, SetVariable)` no longer matches a traced `dict_keys`. ## Notes - Rebased onto #193580 as requested. The earlier revision introduced its own `AbstractSetVariable` bottom tier; that class is gone and `BaseSetVariable` plays its role. - Authored with AI assistance. X-link: https://github.com/pytorch/pytorch/pull/193649 Approved by: https://github.com/guilhermeleobas Reviewed By: huydhn Differential Revision: D118560182 fbshipit-source-id: 712038f47613b60c0243126a5c692a7079beea4f Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Author
generatedunixname499836121
Committer
Parents
Loading