-
Notifications
You must be signed in to change notification settings - Fork 54
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
[IR] Implement register_initializer #1941
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1824,6 +1824,42 @@ | |
def initializers(self) -> dict[str, Value]: | ||
return self._initializers | ||
|
||
def register_initializer(self, value: Value) -> None: | ||
"""Register an initializer to the graph. | ||
|
||
This is a convenience method to register an initializer to the graph with | ||
checks. | ||
|
||
Args: | ||
value: The :class:`Value` to register as an initializer of the graph. | ||
It must have its ``.const_value`` set. | ||
|
||
Raises: | ||
ValueError: If a value of the same name that is not this value | ||
is already registered. | ||
ValueError: If the value does not have a name. | ||
ValueError: If the initializer is produced by a node. | ||
ValueError: If the value does not have its ``.const_value`` set. | ||
""" | ||
if value.name in self._initializers: | ||
if self._initializers[value.name] is not value: | ||
raise ValueError( | ||
f"Initializer '{value.name}' is already registered, but" | ||
" it is not the same object: existing={self._initializers[value.name]!r}," | ||
f" new={value!r}" | ||
) | ||
if not value.name: | ||
raise ValueError(f"Initializer must have a name: {value!r}") | ||
if value.producer() is not None: | ||
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. I wonder if we should support promotion of a 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. I think that can be a graph pass? 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. |
||
raise ValueError( | ||
f"Value '{value!r}' is produced by a node and cannot be an initializer." | ||
) | ||
if value.const_value is None: | ||
raise ValueError( | ||
f"Value '{value!r}' must have its const_value set to be an initializer." | ||
) | ||
self._initializers[value.name] = value | ||
|
||
@property | ||
def doc_string(self) -> str | None: | ||
return self._doc_string | ||
|
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.
is
comparison above with a value-equality check. May not be that important if we add a separate utility to reuse initializers as above, but if not this could serve so approximately.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 think this can be a graph pass too (dedup initializers)
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.
#1943