Skip to content

Conversation

@limintang
Copy link
Contributor

Differential Revision: D65694854

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 9, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 655b391 with merge base c726a9b (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 9, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

limintang added a commit to limintang/executorch that referenced this pull request Nov 9, 2024
…torch#6745)

Summary: Pull Request resolved: pytorch#6745

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

limintang added a commit to limintang/executorch that referenced this pull request Nov 13, 2024
…torch#6745)

Summary: Pull Request resolved: pytorch#6745

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

limintang added a commit to limintang/executorch that referenced this pull request Nov 18, 2024
…torch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

@limintang
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

@cccclai
Copy link
Contributor

cccclai commented Nov 22, 2024

@chiwwang mind providing some suggestion on this PR?

@chiwwang
Copy link
Contributor

chiwwang commented Nov 25, 2024

Hi @limintang ,
Could you please share what use-case this PR aim at?
I'm thinking the context binary can be obtained from the .pte file. Or, if we want a pure context-binary without wrapping it into .pte file, we can also write out the file at AOT compilation stage, i.e., add something like fp.write before this line:

return PreprocessResult(

The qnn_context_binary should be exactly the QNN context-binary loaded by the runtime.

@limintang
Copy link
Contributor Author

limintang commented Nov 26, 2024

Hi @limintang , Could you please share what use-case this PR aim at? I'm thinking the context binary can be obtained from the .pte file. Or, if we want a pure context-binary without wrapping it into .pte file, we can also write out the file at AOT compilation stage, i.e., add something like fp.write before this line:

return PreprocessResult(

The qnn_context_binary should be exactly the QNN context-binary loaded by the runtime.

We frequently use tools in QNN SDK for various debug and profiling tasks, and these tools consume QNN context binary. We are working with QC to support functionalities provided by these tools programmably so we don't have to rely on tool binaries in the future, but until then, saving the context binary in model export is more convenient.

qnn_preprocess.py is another place where the context binary can be written out, actually I also use this path internally for context binary saving. However, the invocation to this module is from ExecuTorch tracing, I'm not sure whether introducing a platform specific config in the platform agnostic ET module is a good idea.

Copy link
Contributor

@chiwwang chiwwang left a comment

Choose a reason for hiding this comment

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

Hi @limintang

Thanks for the response! It makes sense for me.

Just a further question -- do you think environment variable can be a better idea than coupling profiling_level with this need?

I though adding an option to QnnExecuTorchHtpBackendOptions but realized that it's not applicable.
Then I think it would be good if we have runtime option to control this functionality, so environment variable seems a choice.

How about defining an env variable like ET_QNN_BE_DUMP_CNTX_BIN_DIR, if it is not empty, then we dump the context-binaries to the directory it points to?
Is setting env variable possible in your use-case?

limintang added a commit to limintang/executorch that referenced this pull request Nov 27, 2024
…torch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

limintang added a commit to limintang/executorch that referenced this pull request Nov 27, 2024
…torch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

limintang added a commit to limintang/executorch that referenced this pull request Nov 27, 2024
…s set (pytorch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

@limintang limintang changed the title Write QNN context binary to file when profiling level is detailed Write QNN context binary to file when env variable ET_QNN_DEBUG_DIR is set Nov 27, 2024
Copy link
Contributor

@chiwwang chiwwang left a comment

Choose a reason for hiding this comment

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

Looks good for me!

limintang added a commit to limintang/executorch that referenced this pull request Nov 27, 2024
…s set (pytorch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

limintang added a commit to limintang/executorch that referenced this pull request Nov 27, 2024
…s set (pytorch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

@limintang limintang requested a review from shewu-quic November 27, 2024 19:43
…s set (pytorch#6745)

Summary: Pull Request resolved: pytorch#6745

Reviewed By: billmguo

Differential Revision: D65694854
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65694854

@cccclai
Copy link
Contributor

cccclai commented Nov 29, 2024

Sorry a bit late on actually reading this PR...

From reading the conversation, looks like it's mainly to save the qnn_context_binary file, how about we provide a cli tool to unwrap the .pte file to get the qnn_context_binary`, in this case, it can be more generalized. @limintang what do you think?

@limintang
Copy link
Contributor Author

Sorry a bit late on actually reading this PR...

From reading the conversation, looks like it's mainly to save the qnn_context_binary file, how about we provide a cli tool to unwrap the .pte file to get the qnn_context_binary`, in this case, it can be more generalized. @limintang what do you think?

It would be great if you can provide such tool, then this diff won't be necessary.

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. fb-exported topic: not user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants