Skip to content

Conversation

@Erik-Lundell
Copy link
Collaborator

If model-explorer is installed, run it on the exported_graph using ArmTester.visualize(), or use the api the visualize module directly from the debug console.

Introduces two pytest configurations:
--model_explore_host : if set, tries connecting to to a running server
rather than starting a new one.
--model_explore_port : set the port of the above host. If not set, uses default model-explorer port option which searches for a running server.

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 11, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit d98cfe6 with merge base 7fcd0af (image):
💚 Looks good so far! There are no failures yet. 💚

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 Nov 11, 2024
@Erik-Lundell Erik-Lundell added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm ciflow/trunk and removed ciflow/trunk labels Nov 11, 2024
If model-explorer is installed, run it on the exported_graph
using ArmTester.visualize(), or use the api the visualize module
directly from the debug console.

Introduces two pytest configurations:
--model_explore_host : if set, tries connecting to to a running server
	rather than starting a new one.
--model_explore_port : set the port of the above host

Signed-off-by: Erik Lundell <erik.lundell@arm.com>
Change-Id: I00ada14f27e6a7ad3994a439ba4c1e39b1560e2c
_model_explorer_installed = False

try:
# pyre-ignore[21]: We keep track of whether import succeeded manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you just test locally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this has only been tested locally. I am not sure how to test in ci since it requires an external dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

@Olivia-liu - what do you think of this vs. our SDK plans for visualization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bump @Olivia-liu, some kind of visualization would be great, so would really appreciate merging unless you have other plans soon

Copy link
Contributor

Choose a reason for hiding this comment

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

@Erik-Lundell Sorry about the delay! This is great. Glad to see integration with model-explorer going on!

@digantdesai
Copy link
Contributor

Let me get back to you @Erik-Lundell - apologies for the slow responses here.

@digantdesai
Copy link
Contributor

@Olivia-liu - do you think we should make this a devtools utility which can be leveraged by other delegates and/or ET itself? Rationale is, such shared facilities tend to get better faster overtime.

@Olivia-liu
Copy link
Contributor

@Olivia-liu - do you think we should make this a devtools utility which can be leveraged by other delegates and/or ET itself? Rationale is, such shared facilities tend to get better faster overtime.

@digantdesai I agree. Since this visualization isn't Arm specific, it makes sense to make it a shared utility. @Erik-Lundell Would you be able to move it under executorch/devtools? You can create a new directory executorch/devtools/visualization.

Copy link
Contributor

@Olivia-liu Olivia-liu left a comment

Choose a reason for hiding this comment

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

To change it to a shared utility that's not Arm delegate specific

@Erik-Lundell
Copy link
Collaborator Author

Erik-Lundell commented Dec 6, 2024

The idea here was to make it super convenient to use with our tester - the api for just visualizing an exported program in model-explorer is very minimal (basically just model_explorer.visualize_pytorch(exported_program, *options). The project has fixed a bug regarding reuse_server since this PR which this PR had to work around which makes it seem a bit more involved) so I don't think it makes sense to create a utility around it.

What I could do is add it to the xnnpack tester which we inherit from. But I also think I need some guidance on adding model-explorer as a new dependency, can I add it as an optional devtool dependency, should I handle if it is not installed, or something else.

@digantdesai
Copy link
Contributor

I am not a devtools expert, @Olivia-liu is, but how I imagine this as,
(1) visualize(exported_program) should be a devtools utility - including all of visualize.py here
(2) This should not be any delegate specific
(3) This should not be limited to testers
(4) we should make sure it works smoothly i.e. we install right packages with ET setup, have proper docs, etc.

I remember Olivia had something like this but not on the Github yet, so just want to make sure we are aligned and not doing throwaway work here.

Also not want to block you @Erik-Lundell - so I would say just move this out of arm dir to devtools and get a stamp from @Olivia-liu .

@Erik-Lundell Erik-Lundell marked this pull request as draft December 10, 2024 15:46
@Erik-Lundell
Copy link
Collaborator Author

I am not a devtools expert, @Olivia-liu is, but how I imagine this as, (1) visualize(exported_program) should be a devtools utility - including all of visualize.py here (2) This should not be any delegate specific (3) This should not be limited to testers (4) we should make sure it works smoothly i.e. we install right packages with ET setup, have proper docs, etc.

I remember Olivia had something like this but not on the Github yet, so just want to make sure we are aligned and not doing throwaway work here.

Also not want to block you @Erik-Lundell - so I would say just move this out of arm dir to devtools and get a stamp from @Olivia-liu .

Thanks, I have some idea of how to go forward with this now. It'll take some time until I can make the changes, I'll mark the PR as a draft until then.

This was referenced Jan 8, 2025
@Erik-Lundell
Copy link
Collaborator Author

Since the the new patch was completely different I made a new PR, see #7554 @Olivia-liu

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

Labels

ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants