Serialize any node as postponed constant #32490
Conversation
| { | ||
| // Collect all nodes with postponed_constant attribute (not for exclusion, but as starting points) | ||
| std::unordered_set<ov::Node*> postponed_constant_nodes; | ||
| for (const auto& node : sorted_ops) { | ||
| if (node->get_rt_info().count("postponed_constant")) { | ||
| postponed_constant_nodes.insert(node.get()); | ||
| } | ||
| } | ||
|
|
||
| // Perform reverse DFS to find nodes that only feed into postponed_constant nodes | ||
| std::function<void(ov::Node*)> reverse_dfs = [&](ov::Node* node) { | ||
| // Skip if it's a Parameter (model input) | ||
| if (ov::op::util::is_parameter(node)) { | ||
| return; | ||
| } | ||
|
|
||
| // Check if ALL outputs go to postponed_constant or already excluded nodes | ||
| bool all_outputs_excluded = true; | ||
| for (const auto& output : node->outputs()) { | ||
| for (const auto& target_input : output.get_target_inputs()) { | ||
| auto* target_node = target_input.get_node(); | ||
| if (!postponed_constant_nodes.count(target_node) && !nodes_to_exclude.count(target_node)) { | ||
| all_outputs_excluded = false; | ||
| break; | ||
| } | ||
| } | ||
| if (!all_outputs_excluded) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // If all outputs are excluded, mark this node and continue DFS | ||
| if (all_outputs_excluded && node->get_output_size() > 0) { | ||
| nodes_to_exclude.insert(node); | ||
| node->get_rt_info()["disabled_for_serialization"] = true; | ||
|
|
||
| // Recursively process all input nodes | ||
| for (const auto& input : node->inputs()) { | ||
| reverse_dfs(input.get_source_output().get_node()); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Start reverse DFS from all postponed_constant nodes | ||
| for (const auto& node : sorted_ops) { | ||
| if (node->get_rt_info().count("postponed_constant")) { | ||
| for (const auto& input : node->inputs()) { | ||
| reverse_dfs(input.get_source_output().get_node()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could it be moved to separate function? XmlSerializer::serialize is already oversized.
There was a problem hiding this comment.
All comment like // If all outputs are excluded, mark this node and continue DFS suggest that code could be extracted maybe not all as class members but some local helpers.
The serializer is used also by plugins to export compiled model e.g. CPU, also it wil be used by NPU mayeb this feature is not required at all in this cases and there should be possible to disable it at all e.g. by flag or override function
olpipi
changed the title
olpipi
force-pushed
the
postponed_constant_ext
branch
from
17250ca to
37d97d8
Compare
olpipi
changed the title
olpipi
removed
pr: needs tests
|
@praasz , I think the better fix is #32502, are you sure that we need to merge the temp solution? For CF is safer fix current solution this solution |
as this is internal improvement we do prefer to go with this PR, there will be more to improve for next release, but also there will be more space to prepare proper solution. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements serialization support for nodes marked as "postponed constants" by converting them to actual constants during model serialization and excluding their input subgraphs from the serialized output.
Key Changes:
- Added logic to identify nodes marked with
postponed_constantRT info and trace back through their input dependencies - Implemented subgraph exclusion for nodes that only feed into postponed constants
- Modified edge serialization to handle port remapping for postponed constant nodes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/core/tests/pass/serialization/custom_ops.cpp | Adds comprehensive test coverage for postponed constant serialization including concat operations, subgraph exclusion (skipped), multiple consumers, and parameter validation |
| src/core/src/xml_util/xml_serialize_util.cpp | Implements core serialization logic with DFS-based subgraph exclusion and edge filtering for postponed constants |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Skip if it's a Parameter (model input) | ||
| if (ov::op::util::is_parameter(node)) { | ||
| return; |
There was a problem hiding this comment.
The check for Parameter nodes should include an exception throw when a Parameter is encountered in a postponed constant's dependency chain. Test ParameterNotExcluded expects an exception (line 313), but this implementation silently returns, preventing the exception from being raised.
| // Skip if it's a Parameter (model input) | |
| if (ov::op::util::is_parameter(node)) { | |
| return; | |
| // Throw if it's a Parameter (model input) in the dependency chain of a postponed constant | |
| if (ov::op::util::is_parameter(node)) { | |
| OPENVINO_THROW("Parameter node encountered in the dependency chain of a postponed constant, which is not allowed."); |
| } | ||
|
|
||
| // If all outputs are excluded, mark this node and continue DFS | ||
| if (all_outputs_excluded && node->get_output_size() > 0) { |
There was a problem hiding this comment.
The condition node->get_output_size() > 0 is checked after iterating over node->outputs() (line 899). If a node has zero outputs, the loop never executes and all_outputs_excluded remains true, but the node won't be excluded. This creates inconsistent behavior where nodes without outputs aren't excluded even though they have no consumers.
| if (all_outputs_excluded && node->get_output_size() > 0) { | |
| if (all_outputs_excluded) { |
| // Expected: const1, const2 used for Add -> [6,8,10,12] | ||
| // Then multiply by const2 [5,6,7,8] -> [30,48,70,96] | ||
| // Then concat with const2 [5,6,7,8] along axis 0 -> [30,48,70,96,5,6,7,8] reshaped to {4,2} |
There was a problem hiding this comment.
The expected computation comment is incorrect. The Add operation [1,2,3,4] + [5,6,7,8] produces [6,8,10,12], but the Multiply by [5,6,7,8] should yield [30,48,70,96]. However, the comment states this incorrectly as the result matches the provided expected values.
79741b3
5 participants
Details:
Tickets: