[devtool] make ETDumpGen use datasink #8499

Gasoonjia · 2025-02-14T08:38:11Z

Stack from ghstack (oldest at bottom):

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink.

Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r

Differential Revision: D69647096

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) [ghstack-poisoned]

pytorch-bot · 2025-02-14T08:38:16Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❌ 2 New Failures

As of commit 4328491 with merge base cc3974f ():

NEW FAILURES - The following jobs have failed:

pull / test-llava-runner-linux / linux-job (gh)
test_generated_output
pull / unittest / macos / macos-job (gh)
exir/tests/test_joint_graph.py::TestJointGraph::test_joint_graph

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

facebook-github-bot · 2025-02-14T08:38:25Z

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

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) ghstack-source-id: 266455642 Pull Request resolved: #8499

Gasoonjia · 2025-02-14T08:39:45Z

@pytorchbot label "topic: not user facing"

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) [ghstack-poisoned]

Pull Request resolved: #8499 This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r ghstack-source-id: 266706035 @exported-using-ghexport Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/)

facebook-github-bot · 2025-02-16T08:35:11Z

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

facebook-github-bot · 2025-02-17T08:46:22Z

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

facebook-github-bot · 2025-02-17T09:00:02Z

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

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) [ghstack-poisoned]

facebook-github-bot · 2025-02-17T09:01:16Z

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

Pull Request resolved: #8499 This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r ghstack-source-id: 266774207 @exported-using-ghexport Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/)

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) [ghstack-poisoned]

facebook-github-bot · 2025-02-17T23:56:31Z

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

Pull Request resolved: #8499 This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r ghstack-source-id: 266868050 @exported-using-ghexport Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/)

tarun292 · 2025-02-18T19:00:32Z

devtools/etdump/buffer_data_sink.h

   *
   * @param[in] buffer A Span object representing the buffer where data will be
   * stored.
   */
  explicit BufferDataSink(::executorch::runtime::Span<uint8_t> buffer)
      : debug_buffer_(buffer), offset_(0) {}

+  /**


Are there internal users who need this?

not right now but introducing this constructor may make our friends life easier and I'd like to make it as a primary entrance in the future

If our user contructs BufferDataSink from Span:

void* ptr = malloc(2048) Span<uint8_t> span= Span(ptr, 2048) BufferDataSink bds(span)

However if from the new constructor:

void* ptr = malloc(2048) BufferDataSink bds(ptr, 2048)

More straightforward and sturcure, user doesn't need to take care about span or anything else.

I think that the Span ctor would work with syntax like

void* ptr = malloc(2048); BufferDataSink bds({ptr, 2048});

Program syntactically yes, but user need to figure out what's span is, how to construct it, and how to make the logic shorter, which introduces lots of ramp up cost.
Put into user's shoe, they don't need to knwo what span is. A buffer data sink should be directly constructed on the user-provided memory space, and all type transforming should be happened under-the-neath.

tarun292 · 2025-02-18T19:16:05Z

devtools/etdump/etdump_flatcc.cpp

@@ -497,27 +499,15 @@ ETDumpResult ETDumpGen::get_etdump_data() {
 }

 void ETDumpGen::set_debug_buffer(Span<uint8_t> buffer) {


I think we should remove the set_debug_buffer api. Is there any reason to keep it around? Makes it a little confusing for users i think to have these 2 around.

just for BC. Currently all our users use set_debug_buffer to set the memory for debug data, and their work will be impacted if we remove that

Actually we should keep this API around i think. Only users who want to do their custom thing should know about data sinks, otherwise most users should just be able to call set_debug_buffer.

User only need standard data dumping pipeline can use both datasink and set_debug_buffer, since we have provided BufferDataSink which is exactly the same as debug buffer pipeline. But from API design prospective, I would like to deprecate set_debug_buffer api and only focus on data sink for better structure.

tarun292 · 2025-02-18T19:20:02Z

devtools/etdump/etdump_flatcc.cpp

@@ -497,27 +499,15 @@ ETDumpResult ETDumpGen::get_etdump_data() {
 }

 void ETDumpGen::set_debug_buffer(Span<uint8_t> buffer) {
-  debug_buffer_ = buffer;
+  data_sink_ = std::make_shared<BufferDataSink>(buffer);


Curious why make_shared here. This is only owned internally by this class right?

just the underlying data_sink_ is a shared_ptr;

Our user may still want to use theirdata_sink after ETDumpGen lifecycle, so that I make data_sink_ as shared_ptr.

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) [ghstack-poisoned]

facebook-github-bot · 2025-02-18T21:49:49Z

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

Pull Request resolved: #8499 This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r ghstack-source-id: 267038962 @exported-using-ghexport Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/)

tarun292 · 2025-02-19T08:18:09Z

devtools/etdump/etdump_flatcc.cpp

+}
+
+long ETDumpGen::write_tensor_or_raise_error(Tensor tensor) {
+  ET_CHECK_MSG(data_sink_, "Must set data sink before writing data\n");


nit : don't need \n.

This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/) [ghstack-poisoned]

facebook-github-bot · 2025-02-19T08:32:52Z

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

Pull Request resolved: #8499 This diff enables customized debug data pipeline by making ETDumpGen leverage user-provided datasink. Details can be found in https://docs.google.com/document/d/1y_m32mKdj-OgLcLUz9TKhBW3PC3bBDYSBbeAH544EfM/edit?tab=t.0#heading=h.jlkcrurw482r ghstack-source-id: 267135769 @exported-using-ghexport Differential Revision: [D69647096](https://our.internmc.facebook.com/intern/diff/D69647096/)

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 Feb 14, 2025

Gasoonjia mentioned this pull request Feb 14, 2025

[devtool] introduce datasink class to etdump #8496

Open

facebook-github-bot added the fb-exported label Feb 14, 2025

pytorch-bot bot added the topic: not user facing label Feb 14, 2025

Gasoonjia mentioned this pull request Feb 17, 2025

[devtool] make datasink as a sepreate directory #8514

Open

tarun292 reviewed Feb 18, 2025

View reviewed changes

tarun292 reviewed Feb 19, 2025

View reviewed changes

tarun292 approved these changes Feb 19, 2025

View reviewed changes

Gasoonjia mentioned this pull request Feb 20, 2025

[devtool] create stream_data_sink #8604

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[devtool] make ETDumpGen use datasink #8499

[devtool] make ETDumpGen use datasink #8499

Gasoonjia commented Feb 14, 2025 •

edited

Loading

pytorch-bot bot commented Feb 14, 2025 •

edited

Loading

facebook-github-bot commented Feb 14, 2025

Gasoonjia commented Feb 14, 2025

facebook-github-bot commented Feb 16, 2025

facebook-github-bot commented Feb 17, 2025

facebook-github-bot commented Feb 17, 2025

facebook-github-bot commented Feb 17, 2025

facebook-github-bot commented Feb 17, 2025

tarun292 Feb 18, 2025

Gasoonjia Feb 18, 2025

dbort Feb 19, 2025

Gasoonjia Feb 20, 2025

tarun292 Feb 18, 2025

Gasoonjia Feb 18, 2025

tarun292 Feb 19, 2025

Gasoonjia Feb 19, 2025

tarun292 Feb 18, 2025

Gasoonjia Feb 18, 2025

Gasoonjia Feb 18, 2025

facebook-github-bot commented Feb 18, 2025

tarun292 Feb 19, 2025

facebook-github-bot commented Feb 19, 2025

		@@ -497,27 +499,15 @@ ETDumpResult ETDumpGen::get_etdump_data() {
		}

		void ETDumpGen::set_debug_buffer(Span<uint8_t> buffer) {

[devtool] make ETDumpGen use datasink #8499

Are you sure you want to change the base?

[devtool] make ETDumpGen use datasink #8499

Conversation

Gasoonjia commented Feb 14, 2025 • edited Loading

pytorch-bot bot commented Feb 14, 2025 • edited Loading

🔗 Helpful Links

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

❌ 2 New Failures

facebook-github-bot commented Feb 14, 2025

Gasoonjia commented Feb 14, 2025

facebook-github-bot commented Feb 16, 2025

facebook-github-bot commented Feb 17, 2025

facebook-github-bot commented Feb 17, 2025

facebook-github-bot commented Feb 17, 2025

facebook-github-bot commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 18, 2025

Choose a reason for hiding this comment

facebook-github-bot commented Feb 19, 2025

Gasoonjia commented Feb 14, 2025 •

edited

Loading

pytorch-bot bot commented Feb 14, 2025 •

edited

Loading