Skip to content

feat: added checks for support of activations on target#18102

Open
novak-vaclav wants to merge 1 commit intopytorch:mainfrom
nxp-upstream:feature/EIEX-735-add-activation-support-checks
Open

feat: added checks for support of activations on target#18102
novak-vaclav wants to merge 1 commit intopytorch:mainfrom
nxp-upstream:feature/EIEX-735-add-activation-support-checks

Conversation

@novak-vaclav
Copy link
Contributor

@novak-vaclav novak-vaclav commented Mar 11, 2026

Summary

Introduces stricter checks for activation support on the target.
The updated validation logic applies to: clamp, hardtanh, leaky_relu, relu, sigmoid, tanh.

Test plan

tests can be manually run using pytest -c /dev/null backends/nxp/tests/

cc @robert-kalmar @JakeStevens @digantdesai @MartinPavella @roman-janik-nxp

Copilot AI review requested due to automatic review settings March 11, 2026 14:22
@pytorch-bot
Copy link

pytorch-bot bot commented Mar 11, 2026

🔗 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, 11 New Failures, 1 Cancelled Job

As of commit 4e97a6e with merge base 495eec7 (image):

AWAITING APPROVAL - The following workflow needs approval before CI can run:

NEW FAILURES - The following jobs have failed:

CANCELLED JOB - The following job was cancelled. Please retry:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 11, 2026
@novak-vaclav
Copy link
Contributor Author

@pytorchbot label "release notes: nxp"

@novak-vaclav
Copy link
Contributor Author

@pytorchbot label "module: nxp"

@pytorch-bot pytorch-bot bot added release notes: nxp Changes to the NXP Neutron backend delegate module: nxp Issues related to NXP Neutron NPU delegation and code under backends/nxp/ labels Mar 11, 2026
@MartinPavella MartinPavella requested review from MartinPavella and removed request for Copilot March 11, 2026 14:28
num_macs = neutron_target_spec.get_num_macs()

# activations in Neutron are delegable only
# if `num_channels` % `num_macs` == 0 and `num_batches` == 1
Copy link
Collaborator

@MartinPavella MartinPavella Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@novak-vaclav novak-vaclav Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: nxp Issues related to NXP Neutron NPU delegation and code under backends/nxp/ release notes: nxp Changes to the NXP Neutron backend delegate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants