-
Couldn't load subscription status.
- Fork 2.8k
[PoC] Serialize any node as postponed constant - 2nd approach #32502
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
Conversation
ac37557 to
24ce342
Compare
24ce342 to
580ff3f
Compare
| const TensorNamesMap& outputs_names = {}); | ||
|
|
||
|
|
||
| OPENVINO_API std::shared_ptr<ov::Node> make_postponed_constant_from_node(std::shared_ptr<ov::Node> node); |
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.
just a kind reminder to add the short description and the new tests ;)
|
|
||
| class PostponedConstant : public ov::op::Op { | ||
| public: | ||
| OPENVINO_OP("Constant", "opset1"); |
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.
Mimic-ing Constant in such way is risky. Note that ov::is_type uses this and it may lead to issues elsewhere - ov::op::v0::Constant has exactly same definition.
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.
Pull Request Overview
This PR introduces a mechanism to serialize any node as a postponed constant, which delays constant folding until model serialization time. The implementation creates a special PostponedConstant operation that wraps a node and presents itself as a Constant op from opset1, but defers actual constant evaluation until the model is saved to IR format.
Key changes:
- Implements
PostponedConstantclass that wraps arbitrary nodes and masquerades as Constants - Adds factory function to create and replace nodes with postponed constants
- Exposes the factory function through the public API header
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/core/src/op/util/posponed_constant.cpp | Implements PostponedConstant operation and factory function |
| src/core/dev_api/openvino/core/model_util.hpp | Adds public API declaration for postponed constant creation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| std::shared_ptr<Node> clone_with_new_inputs(const ov::OutputVector& inputs) const { | ||
| OPENVINO_THROW("PostponedConstant cannot be copied"); |
Copilot
AI
Oct 22, 2025
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 clone_with_new_inputs method throws an exception unconditionally, but it doesn't verify or document that inputs should be empty (since PostponedConstant has no inputs). Consider adding a parameter validation and update the error message to be more descriptive: 'PostponedConstant cannot be cloned with new inputs'.
| OPENVINO_THROW("PostponedConstant cannot be copied"); | |
| if (!inputs.empty()) { | |
| OPENVINO_THROW("PostponedConstant cannot be cloned with new inputs; expected no inputs."); | |
| } | |
| OPENVINO_THROW("PostponedConstant cannot be cloned."); |
| ov::OutputVector outputs(1); | ||
| OPENVINO_ASSERT( | ||
| m_node->constant_fold(outputs, m_node->input_values()), | ||
| "Node with set `postponed_constant` attribute cannot be fold to constant when saving model to IR file"); |
Copilot
AI
Oct 22, 2025
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 error message has a grammatical error. Change 'cannot be fold' to 'cannot be folded'.
| "Node with set `postponed_constant` attribute cannot be fold to constant when saving model to IR file"); | |
| "Node with set `postponed_constant` attribute cannot be folded to constant when saving model to IR file"); |
| constructor_validate_and_infer_types(); | ||
| }; | ||
|
|
||
| void validate_and_infer_types() { |
Copilot
AI
Oct 22, 2025
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 validate_and_infer_types method should be marked as override since it overrides a virtual method from the base Op class.
| void validate_and_infer_types() { | |
| void validate_and_infer_types() override { |
| set_output_type(0, output.get_element_type(), output.get_partial_shape()); | ||
| } | ||
|
|
||
| std::shared_ptr<Node> clone_with_new_inputs(const ov::OutputVector& inputs) const { |
Copilot
AI
Oct 22, 2025
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 clone_with_new_inputs method should be marked as override since it overrides a virtual method from the base Op class.
| std::shared_ptr<Node> clone_with_new_inputs(const ov::OutputVector& inputs) const { | |
| std::shared_ptr<Node> clone_with_new_inputs(const ov::OutputVector& inputs) const override { |
| OPENVINO_THROW("PostponedConstant cannot be copied"); | ||
| } | ||
|
|
||
| bool visit_attributes(AttributeVisitor& visitor) { |
Copilot
AI
Oct 22, 2025
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 visit_attributes method should be marked as override since it overrides a virtual method from the base Op class.
| bool visit_attributes(AttributeVisitor& visitor) { | |
| bool visit_attributes(AttributeVisitor& visitor) override { |
| OPENVINO_API std::shared_ptr<ov::Node> make_postponed_constant_from_node(std::shared_ptr<ov::Node> node); | ||
|
|
Copilot
AI
Oct 22, 2025
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 new public API function make_postponed_constant_from_node lacks documentation. Add a docstring explaining the function's purpose, parameters, return value, and any preconditions (e.g., that the node must have exactly one output).
| OPENVINO_API std::shared_ptr<ov::Node> make_postponed_constant_from_node(std::shared_ptr<ov::Node> node); | |
| /** | |
| * @brief Creates a postponed constant node from the given node. | |
| * | |
| * This function generates a constant node whose value is determined at a later stage, | |
| * based on the output of the provided node. The input node must have exactly one output. | |
| * | |
| * @param node A shared pointer to the node from which to create the postponed constant. | |
| * The node must have exactly one output. | |
| * @return A shared pointer to the newly created postponed constant node. | |
| * | |
| * @note The input node must have exactly one output. | |
| */ | |
| OPENVINO_API std::shared_ptr<ov::Node> make_postponed_constant_from_node(std::shared_ptr<ov::Node> node); |
|
I'm concerning about the path (src/core/src/op/util/posponed_constant.cpp), @mitruska could you advise the right place for the custom operations? |
I assume the location is intentional, as it makes it hidden from public API and possible to used with this dedicated helper only. |
Details:
Tickets: