feat: added checks for support of activations on target#18102
feat: added checks for support of activations on target#18102novak-vaclav wants to merge 1 commit intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18102
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 Awaiting Approval, 7 New FailuresAs of commit f75ac40 with merge base 495eec7 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
|
@pytorchbot label "release notes: nxp" |
|
@pytorchbot label "module: nxp" |
pytorch-bot
bot
added
release notes: nxp
MartinPavella
requested review from
MartinPavella
and removed request for
Copilot
| num_macs = neutron_target_spec.get_num_macs() | ||
|
|
||
| # activations in Neutron are delegable only | ||
| # if `num_channels` % `num_macs` == 0 and `num_batches` == 1 |
There was a problem hiding this comment.
This is incorrect.
You call this function in every activation converter's is_supported_on_target() method. So this condition has to always be satisfied to delegate an activation. But operators which can use the Custom Activation feature of Neutron aren't restricted like this.
For example test_leaky_relu_converter.py contained tests with input shape (23, ), and it delegated successfully.
Your task was aimed only at solving the issue when the activations were the only ops in the model. But here you are introducing a check which effects every case (including models with multiple nodes), and it breaks existing working functionality.
I see you changed all existing tests to satisfy this condition. But the tests were passing, and they should still be passing if Neutron supports them.
Our goal is to delegate as many operators to Neutron as possible. Therefore, we cannot introduce arbitrary restrictions like this. Every single test that started failing when you introduced this change must still be passing. You cannot just modify the tests and pretend it didn't happen. If a test fails -> there is an issue. In this case, the issue was caused by your changes.
It appears you have misunderstood the task. In clamp_converter.py, you have removed the method supports_partitioning_result(). What was the motivation for this? Instead, you should have added this method to every "activation_converter" and used it to prohibit delegation if the activation was the only node in the partition (and perhaps consider some operator-specific conditions).
Unless I'm missing something, a complete rework is required.
There was a problem hiding this comment.
As I told you and the team multiple times at the regular meetups, I noticed that the converters did NOT check whether the node is delegable. Such functionality was not implemented, when in my opinion it should have been. I also believe that the absence of such functionality was the root cause of ReLU not being delegated when there are no other nodes in the partition (aside from QDQ nodes of course).
In the Neutron Converter documentation, there is a list of conditions, such as num_channels % num_macs == 0 that are not being checked. That is the whole motivation behind all these changes.
The tests needed to be modified to correctly reflect the newly imposed restrictions. I wanted to adhere to the documentation the Neutron team provided, however if you think it is better to start checking the requirements only when we encounter a situation when it stops working, that is fine by me. Just wanted to do the due dilligence of properly implementing the conversion of one of the most important operators there are, such as ReLU or Sigmoid.
As I am looking at it right now, some of the code is quite old, for example the functionality concerning ReLU. Maybe the restrictions did not exist back then.
There was a problem hiding this comment.
Let's not escalate this.
The bottom line is this:
- You wanted to adhere to the Neutron documentation, even in cases when we have working implementation which contradicts the documentation. So you prohibited these cases, which solved the issue of your ticket, but it also broke some supported cases.
- I think we should support as many cases as possible. We know the Neutron documentation is not 100% accurate, so I think prohibiting working cases just because they contradict the docs is not the way to go. Based on this opinion, the solution should have been different.
We can discuss it tomorrow if you want.
P.S.
I didn't mean to sound aggressive in my first comment. I apologize if my phrasing was inappropriate.
There was a problem hiding this comment.
Did major rework, please review the changes.
novak-vaclav
force-pushed
the
feature/EIEX-735-add-activation-support-checks
branch
from
4e97a6e to
f75ac40
Compare
|
New PR description after major rework: Introduces stricter checks for support in cases where there might be |
There was a problem hiding this comment.
Pull request overview
Adds stricter target-specific validation for delegating certain activation operators to the NXP Neutron backend, plus new/updated tests that assert delegation vs non-delegation based on shape/partition context.
Changes:
- Introduces
activation_supported_on_target()to gate delegation of activation-only partitions based on channel-count vs targetNUM_MACS. - Updates ReLU/HardTanh/Clamp converters to apply the new “alone in partition” activation-support validation.
- Expands converter tests to assert activation delegation (or non-delegation) by inspecting the delegated graph.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backends/nxp/tests/models.py | Adds a HardTanhModule helper model for hardtanh-only test cases. |
| backends/nxp/tests/ir/converter/node_converter/test_relu_converter.py | Adds graph-level assertions to verify when ReLU is delegated vs kept. |
| backends/nxp/tests/ir/converter/node_converter/test_hardtanh_converter.py | Updates preprocessing helpers and adds an “unsupported” hardtanh delegation test. |
| backends/nxp/tests/ir/converter/node_converter/test_clamp_converter.py | Adds an “unsupported shape” case for clamp-as-(relu/relu6) when alone in a partition. |
| backends/nxp/backend/neutron_operator_support.py | Adds activation_supported_on_target() helper implementing the channel divisibility check. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/relu_converter.py | Applies activation support check when ReLU is alone in a partition. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/hardtanh_converter.py | Refactors hardtanh mode handling and applies activation support check for Relu/Relu6 modes when alone. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/clamp_converter.py | Replaces previous single-node partition logic with shared “alone in partition” + activation support check. |
| backends/nxp/backend/ir/converter/node_converter.py | Adds is_node_alone_in_partition() helper used by multiple converters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| mocker, | ||
| input_shape: tuple[int], | ||
| activation_range: tuple[int, int], |
| return True | ||
|
|
||
| def convert(self, node: Node): | ||
| """Convert 'aten::hardtanh' to it's supported ReLU equivalent.""" |
| # should not occur | ||
| min_val = 0 | ||
| max_val = 1 |
| def activation_supported_on_target( | ||
| node: Node, neutron_target_spec: NeutronTargetSpec | ||
| ) -> bool: | ||
| """This function determines if the current NeutronSoftware properly supports an activation operator represented by the given node. | ||
|
|
||
| :param node: The node representing the activation operator. | ||
| :param neutron_target_spec: Object for querying the target platform to retrieve its properties. | ||
| """ | ||
| input_shape = list(input_tensor(node, 0).shape) | ||
| if node.args[0].meta[NXP_NODE_FORMAT].is_channels_first(): | ||
| input_shape = dims_to_channels_last(input_shape) | ||
|
|
||
| c = input_shape[-1] | ||
| num_macs = neutron_target_spec.get_num_macs() | ||
|
|
||
| # activations in Neutron are delegable only | ||
| # if `num_channels` % `num_macs` == 0 | ||
| return c % num_macs == 0 |
| @staticmethod | ||
| def is_node_alone_in_partition( | ||
| node: Node, partition_list: list[Partition], filter_fn: Callable | ||
| ): | ||
| """Return True if `node` is the only node in its partition for which `filter_fn` | ||
| returns True. | ||
|
|
||
| The function finds the unique partition containing `node` and applies | ||
| `filter_fn` to all nodes in that partition. If only one node passes the | ||
| predicate — and that node is `node` — the function returns True. | ||
|
|
||
| :param node: The torch.Node to check. | ||
| :param partition_list: List of proposed partitions. | ||
| :param filter_fn: Predicate applied to nodes in the partition. | ||
| `node` is considered alone if it is the only node | ||
| for which this predicate returns True. | ||
| """ | ||
| partitions = [p for p in partition_list if node in p.nodes] | ||
| if len(partitions) != 1: | ||
| return False # Should never happen | ||
|
|
||
| partition = partitions[0] | ||
| filtered_partition_nodes = list(filter(filter_fn, partition.nodes)) | ||
| return ( | ||
| len(filtered_partition_nodes) == 1 and filtered_partition_nodes[0] == node | ||
| ) |
MartinPavella
left a comment
MartinPavella
left a comment
There was a problem hiding this comment.
Very nice solution 👍🏻
I pretty much only have cosmetic suggestions, and Copilot had a couple of good points. With those details implemented, LGTM.
| } | ||
|
|
||
| @staticmethod | ||
| def _get_hardtanh_bounds(node: Node) -> tuple[int, int]: |
There was a problem hiding this comment.
Nit: The return type should be tuple[float, float] as the bounds don't need to be strictly integers.
| _, min_value, max_value = node.args | ||
| return (min_value, max_value) in HardTanhConverter.supported_modes_map.keys() | ||
| bounds = HardTanhConverter._get_hardtanh_bounds(node) | ||
| return bounds in HardTanhConverter.SUPPORTED_MODES_MAP.keys() |
There was a problem hiding this comment.
Nit: The .keys() is redundant.
| if bounds in [ | ||
| cls.SUPPORTED_BOUNDS_MAP["Relu"], | ||
| cls.SUPPORTED_BOUNDS_MAP["Relu6"], | ||
| ]: |
There was a problem hiding this comment.
Nit: A comment explaining why only Relu and Relu6 would be useful.
| @@ -77,18 +80,11 @@ def supports_partitioning_result( | |||
| bounds = cls._get_clamp_bounds(node) | |||
|
|
|||
| if bounds in [cls.SUPPORTED_BOUNDS["Relu"], cls.SUPPORTED_BOUNDS["Relu6"]]: | |||
There was a problem hiding this comment.
Nit: A comment explaining why only Relu and Relu6 would be useful.
| `filter_fn` to all nodes in that partition. If only one node passes the | ||
| predicate — and that node is `node` — the function returns True. | ||
|
|
||
| :param node: The torch.Node to check. |
| """ | ||
| partitions = [p for p in partition_list if node in p.nodes] | ||
| if len(partitions) != 1: | ||
| return False # Should never happen |
There was a problem hiding this comment.
When this code was inside the supports_partitioning_result, returning False would result in not delegating the node. Here, it would have the opposite effect. Perhaps we should raise an exception here instead. What do you think?
| return True | ||
|
|
||
| @staticmethod | ||
| def is_node_alone_in_partition( |
Summary
Introduces stricter checks for activation support on the target.
The updated validation logic applies to:
clamp,hardtanh,leaky_relu,relu,sigmoid,tanh.edit after rework:
Introduces stricter checks for support in cases where there might be
reluorrelu6operator alone in a partition. In such cases, the Neutron converter fails to delegate the operator. Also fixed a small issue inhardtanh_converterrelated to reading out the arguments from the edge operator.This additional check applies for
relu_converter,clamp_converterandhardtanh_converter. Also checked whether the issues does not affect other activations, such asleaky_relu,tanhandsigmoid, but empirically the issue did not occur there.Test plan
tests can be manually run using
pytest -c /dev/null backends/nxp/tests/cc @robert-kalmar @JakeStevens @digantdesai @MartinPavella @roman-janik-nxp