Update MemoryPlanning Verifier to only assume model has user input if it has at least one tensor input#10617
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10617
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New FailureAs of commit 8a87967 with merge base 9d899c9 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Didn't find following labels among repository labels: Release notes: exir |
|
@pytorchbot label "release notes: exir" |
|
Started CI (Need to do for new, external contributions). Rebase please. Also marked it as Ready to signal reviewers that it's, well, ready for a review. Thanks for the contribution. Welcome to Executorch 😊 |
|
Thank you, I just rebased. I also fixed the linting issues which the CI tests identified. [Sorry, I messed this up, hence the subsequent force-push.] |
… it has at least one tensor input MemoryPlanning verifier currently blows up if all the user inputs are prims. This change means its helper function _do_user_inputs_exist returns false if all its inputs are prims. It only returns true if at least one input is a tensor.
|
cc @JacobSzwejbka can you take a look? 🙏 |
|
@JacobSzwejbka gentle ping |
JacobSzwejbka
left a comment
There was a problem hiding this comment.
I dont see any obvious issue with the code, the all red ci though made me think it was still under iteration.
|
Many thanks for looking at this @JacobSzwejbka. Is there anything I can do now to help get this PR closed? |
|
Kicked off CI jobs. |
|
I don't think there's any other comment so approving |
|
I'm so sorry @larryliu0820, I accidentally merged the branch which seems to have stopped the running workflows. Could you please kick off the CI jobs again? |
|
Please correct me if I'm wrong, but I believe the failing check (pull / android / run-emulator (pull_request)) is unrelated to my change. It seems to be failing in other PRs too. For example: has the same failed check here, with the same failure output: What is the best way to proceed to get this merged? |
|
Yeah unrelated. Merging. Thanks for the PR! |
|
Fantastic, thank you so much for your help and patience throughout! This was my first code contribution to a major open-source project. 😄 |
Fixes #10522
Summary
MemoryPlanning verifier currently blows up if all the user inputs are prims. #10522 suggested:
This PR implements this suggestion, with accompanying unit tests.
_do_user_inputs_existnow returns True if it has at least one tensor input, and False otherwise.Test plan
Added unit tests to
test_memory_planning.pyand ran them withPlease note you must comment out the line 69 from
/pytest.inifor this to work:On my machine, I also had to comment out line 60 from
test_memory_planning.pyfor the test to run without errors.[The tests I wrote aren't dependent on this library.]