Skip to content

Comments

Add memory planning to oss tests#17596

Open
lucylq wants to merge 1 commit intomainfrom
lfq.add-memory-planning-to-oss-tests
Open

Add memory planning to oss tests#17596
lucylq wants to merge 1 commit intomainfrom
lfq.add-memory-planning-to-oss-tests

Conversation

@lucylq
Copy link
Contributor

@lucylq lucylq commented Feb 20, 2026

Summary

add to pytest.ini, and add cmake path-finding import for OSS

Test plan

python -m unittest exir.tests.test_memory_planning

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 20, 2026

🔗 Helpful Links

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

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

❌ 4 New Failures

As of commit ad56f3e with merge base d17ff74 (image):

NEW FAILURES - The following jobs have failed:

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 Feb 20, 2026
@github-actions
Copy link

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.

@lucylq lucylq marked this pull request as ready for review February 20, 2026 20:18
Copilot AI review requested due to automatic review settings February 20, 2026 20:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Enables the exir/tests/test_memory_planning.py suite to run in OSS test environments by removing its pytest ignore and adding a non-Buck shared-library discovery path for custom_ops_generated_lib.

Changes:

  • Stop ignoring exir/tests/test_memory_planning.py in pytest.ini so it runs under pytest in OSS.
  • Add a try/except around torch.ops.load_library to fall back to locating the built shared library via filesystem globbing when not running under Buck.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
pytest.ini Un-ignores the memory planning test so it’s included in the OSS pytest run.
exir/tests/test_memory_planning.py Adds fallback logic to locate/load custom_ops_generated_lib outside Buck (e.g., CMake/pip installs).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 33 to 36
# Ignore tests with missing custom_ops_generated_lib dependencies
--ignore=exir/backend/test/demos/test_delegate_aten_mode.py
--ignore=exir/backend/test/demos/test_xnnpack_qnnpack.py
--ignore=exir/tests/test_memory_format_ops_pass_aten.py
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

The addopts section header says these are tests with missing custom_ops_generated_lib dependencies, but exir/tests/test_memory_planning.py is no longer ignored even though it still loads that library. Please update this comment (or move the test out of this section) so the ignore rationale remains accurate.

Copilot uses AI. Check for mistakes.
.glob("**/kernels/portable/**/*custom_ops_generated_lib.*")
)
if _libs:
torch.ops.load_library(str(_libs[0]))
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

If the Buck target load fails and the glob finds no matches, the module import continues without loading custom_ops_generated_lib, which will likely cause later failures in ToOutVarPass (missing out variants) with a less-direct error. Consider failing fast with a clearer error or skipping these tests when the library cannot be located/loaded.

Suggested change
torch.ops.load_library(str(_libs[0]))
torch.ops.load_library(str(_libs[0]))
else:
# Fail fast or explicitly skip if the custom ops library cannot be found.
# This avoids obscure downstream errors (e.g., in ToOutVarPass).
raise unittest.SkipTest(
"Skipping memory planning tests: could not locate "
"custom_ops_generated_lib shared library."
)

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +75
_libs = list(
Path(__file__)
.parent.parent.parent.resolve()
.glob("**/kernels/portable/**/*custom_ops_generated_lib.*")
)
if _libs:
torch.ops.load_library(str(_libs[0]))
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

The recursive glob (**/kernels/portable/**/*custom_ops_generated_lib.*) can be expensive and is also imprecise (it may match non-shared-library artifacts and, when multiple matches exist, _libs[0] is not deterministic). Consider filtering to platform shared-library extensions, sorting the matches, and/or searching only known build/install locations to keep imports fast and deterministic.

Copilot uses AI. Check for mistakes.
--ignore=exir/backend/test/demos/test_xnnpack_qnnpack.py
--ignore=exir/tests/test_memory_format_ops_pass_aten.py
--ignore=exir/tests/test_memory_planning.py

Copy link
Contributor

Choose a reason for hiding this comment

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

lol. Didnt realize we had so much stuff disabled

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants