-
Notifications
You must be signed in to change notification settings - Fork 181
Allow freezing of FunctionGraph for hashing #1908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3
Are you sure you want to change the base?
Changes from all commits
ee229a5
875a0f5
fab4d67
2cfbd00
d5a249b
355e912
19cd75e
c3b23df
418b89f
221e801
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import abc | ||
| import warnings | ||
| import weakref | ||
| from collections.abc import ( | ||
| Hashable, | ||
| Iterable, | ||
|
|
@@ -14,6 +15,7 @@ | |
| Any, | ||
| Generic, | ||
| Optional, | ||
| Self, | ||
| TypeVar, | ||
| Union, | ||
| cast, | ||
|
|
@@ -693,7 +695,7 @@ def _reduce(self): | |
| return cls, (self.id, self.type) | ||
|
|
||
| def _str(self): | ||
| return f"*{self.id}-{var_type.__str__(self)}" | ||
| return f"i{self.id}" | ||
|
|
||
| new_type = type( | ||
| type_name, (cls, var_type), {"__reduce__": _reduce, "__str__": _str} | ||
|
|
@@ -788,6 +790,93 @@ def value(self): | |
| return self.data | ||
|
|
||
|
|
||
| def _get_frozen_output(apply_node: "FrozenApply", index: int) -> Variable: | ||
| """Resolve a FrozenApply output by index. Used by pickle.""" | ||
| return apply_node.outputs[index] | ||
|
|
||
|
|
||
| def _make_frozen_output_reduce(out: Variable): | ||
| """Create a __reduce_ex__ override for a FrozenApply output Variable.""" | ||
| owner = out.owner | ||
| index = out.index | ||
|
|
||
| def __reduce_ex__(protocol): | ||
| return (_get_frozen_output, (owner, index)) | ||
|
|
||
| return __reduce_ex__ | ||
|
|
||
|
|
||
| class FrozenApply(Apply): | ||
| """An immutable, globally-interned Apply node for frozen graphs. | ||
|
|
||
| Uses tuples for ``inputs`` and ``outputs`` so mutation raises ``TypeError`` | ||
| at the language level. Interned by ``(op, cache_key(inputs))`` — | ||
| constructing a ``FrozenApply`` with the same op and input variables returns | ||
| the cached instance. | ||
|
|
||
| Constants are keyed by ``(type, data_bytes)`` so that two independently | ||
| created Constants with the same value resolve to the same cached node. | ||
| """ | ||
|
|
||
| _cache: weakref.WeakValueDictionary = weakref.WeakValueDictionary() | ||
|
|
||
| @staticmethod | ||
| def _input_to_key(inp: Variable): | ||
| """Convert an input Variable to a hashable, value-based cache key element. | ||
|
|
||
| Non-Constants (NominalVariables, FrozenApply outputs) are already | ||
| globally interned, so identity works. Constants use their byte | ||
| representation so that independently-created equal constants | ||
| (including NaN) produce the same key. Object-dtype constants | ||
| (e.g. slices) fall back to ``signature()`` since their byte | ||
| representation stores pointers, not values. | ||
| """ | ||
| if isinstance(inp, Constant): | ||
| a = np.asarray(inp.data) | ||
| if a.dtype.kind != "O": | ||
| return (inp.type, a.tobytes(), a.dtype.str, a.shape) | ||
| return inp.signature() | ||
| return inp | ||
|
|
||
| def __new__( | ||
| cls, | ||
| op: "Op", | ||
| inputs: tuple[Variable, ...], | ||
| output_types: tuple["Type", ...], | ||
| ): | ||
| cache_key = (op, tuple(cls._input_to_key(i) for i in inputs)) | ||
| cached = cls._cache.get(cache_key) | ||
| if cached is not None: | ||
| return cached | ||
|
|
||
| instance = object.__new__(cls) | ||
| instance.op = op | ||
| instance.inputs = inputs # type: ignore[assignment] | ||
| instance.outputs = tuple( # type: ignore[assignment] | ||
| t.variable_type(type=t, owner=instance, index=i) | ||
| for i, t in enumerate(output_types) | ||
| ) | ||
| # Give each output Variable a __reduce__ that resolves to the | ||
| # canonical output on unpickle, avoiding fresh Variable objects. | ||
| for out in instance.outputs: | ||
| out.__reduce_ex__ = _make_frozen_output_reduce(out) # type: ignore[method-assign] | ||
|
Comment on lines
+859
to
+862
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ELI5?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FrozenApply is interned, but it's output variables are not. When we unpickle stuff, python will make a new object so that The patch makes it so that when unpickling, we go look for the (interned) "canonical" version of the output (e.g.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the apply is interned the variables are also (and vice versa). I assumed we were going to intern the variables because you may have variables without apply but not the other way around
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We intern the variables, but only transitively. We have to go reach into the FrozenApply to get them. |
||
| instance.tag = Scratchpad() | ||
| cls._cache[cache_key] = instance | ||
| return instance | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the frozenapply to be hash-consed? Isn't it enough if the input/output variables are? Wondering if we can remove some extra code that way. The Apply doesn't do much anyway
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is tied up in the current identity-based equality scheme. If we remove the FrozenApply interning, FrozenFunctionGraph.init will create new Apply nodes with new output Variables, so we lose That's not to say we couldn't move the equality check responsibility inside FFG, but it's a slightly different design. |
||
|
|
||
| def __init__(self, op, inputs, output_types): | ||
| # All initialization is done in __new__ | ||
| pass | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we want to return cached variables for the outputs. new is run before an object exists, so we have time to do the output injection. If we put the setup in init, on cache hit:
|
||
|
|
||
| def clone(self, clone_inner_graph: bool = False) -> Self: | ||
| """Frozen nodes are immutable — cloning returns self.""" | ||
| return self | ||
|
|
||
| def __reduce__(self): | ||
| output_types = tuple(o.type for o in self.outputs) | ||
| return (type(self), (self.op, self.inputs, output_types)) | ||
|
|
||
|
|
||
| def clone( | ||
| inputs: Sequence[Variable], | ||
| outputs: Sequence[Variable], | ||
|
|
@@ -1104,14 +1193,14 @@ def equal_computations( | |
|
|
||
| for x, y in zip(xs, ys, strict=True): | ||
| if not isinstance(x, Variable) and not isinstance(y, Variable): | ||
| return np.array_equal(x, y) | ||
| return np.array_equal(x, y, equal_nan=True) | ||
| if not isinstance(x, Variable): | ||
| if isinstance(y, Constant): | ||
| return np.array_equal(y.data, x) | ||
| return np.array_equal(y.data, x, equal_nan=True) | ||
| return False | ||
| if not isinstance(y, Variable): | ||
| if isinstance(x, Constant): | ||
| return np.array_equal(x.data, y) | ||
| return np.array_equal(x.data, y, equal_nan=True) | ||
| return False | ||
| x_is_owned, y_is_owned = (x.owner is not None, y.owner is not None) | ||
| if x_is_owned != y_is_owned: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still seems like we're getting a lot of complexity for this FrozenApply thing that is not present in regular interned Variables? Is it due to pickling? Why is it trickier?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of FrozenApply is to allow equality-based comparison of FrozenFunctionGraph outputs. I went through an intermediate plan that used a tuple spec of
(op, input_refs, n_ouputs)per node that would also have worked, but then we're maintaining that machinery. If we want to be able to do fg1 == fg2, we have to have an abstraction somewhere that allows robust serialization/hashing of nodes (including constants, which have been a repeated challenge during this PR)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the variables being hash-consed already achieves that, you could even have stuck with regular Apply objects holding the hash consed variables as inputs/outputs.
I suggested a frozen apply just so it would use tuples and reduce the risk of accidentally mutating them, but they never seemed necessary (to me) for the goal of hash/equality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's not necessary per se, but there has to be some kind of hashable topological representation somewhere so we can rebuild the graph. The tuple-spec or "fingerprint" was one approach that interns the variables directly, and just the whole graph topology in a list of nested tuples. I settled on FrozenApply because it also encodes the same topology but in a representation that feels more "pytensor native". The downside is that it adds this intermediate object that we have to go through to get the variables themselves.