Skip to content

Revert "Arm backend: Split executor runner into init + run" #12171

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 2, 2025
Merged

Revert "Arm backend: Split executor runner into init + run" #12171

merged 1 commit into from
Jul 2, 2025

Conversation

@metascroy
Copy link
Contributor

Reverts #12162

This fails internal CI. @digantdesai can you have a look

@metascroy metascroy requested a review from digantdesai as a code owner July 2, 2025 18:58
@pytorch-bot pytorch-bot bot added the ci-no-td label Jul 2, 2025
@pytorch-bot
Copy link

pytorch-bot bot commented Jul 2, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12171

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 Cancelled Jobs

As of commit f06ab81 with merge base 22b9e59 (image):

CANCELLED JOBS - The following jobs were cancelled. Please retry:

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

@facebook-github-bot facebook-github-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 Jul 2, 2025
@github-actions
Copy link

github-actions bot commented Jul 2, 2025

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@metascroy metascroy merged commit dc7fd75 into main Jul 2, 2025
100 of 104 checks passed
@metascroy metascroy deleted the revert-12162-marlin-executor-runner branch July 2, 2025 21:28
@zingo
Copy link
Collaborator

zingo commented Jul 3, 2025

What errors/problems did you see @metascroy ?

@martinlsm martinlsm mentioned this pull request Jul 3, 2025
Merged
@martinlsm
Copy link
Collaborator

I have now created a new PR (#12197) with this same patch. Would be good to know what the error was. Hopefully it was just a conflict with another patch when merging(?)

zingo pushed a commit that referenced this pull request Jul 7, 2025
@martinlsm
Resubmission of Arm backend: Split executor runner into init + run (#…
487da8c 
…12197)

The PR #12162 got reverted with #12171. This PR is a resubmission of the
patch.

Refactor the main function of arm_executor_runner.cpp by extracting code
that is related to initializing and running a model into two separate
helper functions (runner_init and RunnerContext::run). A new struct
called RunnerContext is introduced to store data/context required to run
the model.

### Test plan
Arm's CI runs and tests the changed code.

Co-authored-by: Martin Lindström <[email protected]>
@digantdesai
Copy link
Contributor

digantdesai commented Jul 9, 2025 •
edited
Loading

@metascroy I saw D77657568 and the failure doesn't seem be related to this file/PR. Do you know why this had to be reverted? Also we relanded this in #12197 - I don't think there was any change, just curious.

Tanish2101 pushed a commit to Tanish2101/executorch that referenced this pull request Jul 9, 2025
@metascroy @Tanish2101
Revert "Arm backend: Split executor runner into init + run" (pytorch#…
853ed14 
…12171)

Reverts pytorch#12162

This fails internal CI.  @digantdesai can you have a look
Tanish2101 pushed a commit to Tanish2101/executorch that referenced this pull request Jul 9, 2025
@martinlsm @Tanish2101
Resubmission of Arm backend: Split executor runner into init + run (p…
24482b8 
…ytorch#12197)

The PR pytorch#12162 got reverted with pytorch#12171. This PR is a resubmission of the
patch.

Refactor the main function of arm_executor_runner.cpp by extracting code
that is related to initializing and running a model into two separate
helper functions (runner_init and RunnerContext::run). A new struct
called RunnerContext is introduced to store data/context required to run
the model.

### Test plan
Arm's CI runs and tests the changed code.

Co-authored-by: Martin Lindström <[email protected]>
@metascroy
Copy link
Contributor Author

@metascroy I saw D77657568 and the failure doesn't seem be related to this file/PR. Do you know why this had to be reverted? Also we relanded this in #12197 - I don't think there was any change, just curious.

I reverted it because the failing internal tests were blocking the difftrain / codesync. Reverting is a preferred way to unblock the difftrain based on our oncall runbook, but in this case it looks like the failures were false positive / transient.

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

Reviewers

@JacobSzwejbka JacobSzwejbka JacobSzwejbka approved these changes

@guangy10 guangy10 guangy10 approved these changes

@digantdesai digantdesai Awaiting requested review from digantdesai digantdesai is a code owner

Assignees

No one assigned

Labels

ci-no-td CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@metascroy @zingo @martinlsm @digantdesai @JacobSzwejbka @guangy10 @facebook-github-bot