Add try-before-set tests for DataSink in DebugEvent and LogDelegateIntermediateOutput #9762
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9762
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit f27d896 with merge base bc3d437 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
@pytorchbot label "topic: not user facing" |
|
Hi @Gasoonjia, would you mind taking a look at my changes to confirm they match what you had in mind? I noticed that the DebugEvent test (starting at line 188) follows the same pattern as LogDelegateIntermediateOutput before I added the additional ET_EXPECT_DEATH checks. Do you think similar checks should be added there as well? |
|
hi @keyprocedure thanks for your great work! |
keyprocedure
changed the title
|
|
||
| auto buffer_data_sink = BufferDataSink::create(ptr, 2048); | ||
| auto buffer_data_sink = BufferDataSink::create(ptr, debug_buf_size); | ||
| auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); | ||
|
|
||
| etdump_gen[i]->create_event_block("test_block"); |
There was a problem hiding this comment.
perhapes we can move this line under if (j == 0) to make more structural.
There was a problem hiding this comment.
Good catch! I moved line 544 under the if (j==0) condition
Does the second comment, "same as above", refer to line 209 in the most recently changed file?
It looks like that call creates a target block where values logged via log_evalue in the following lines are appended
There was a problem hiding this comment.
Overall LGTM! Thanks @keyprocedure for your contribution! Will stamp it after ci pass.
| @@ -497,36 +515,57 @@ TEST_F(ProfilerETDumpTest, VerifyData) { | |||
| } | |||
| } | |||
|
|
|||
| // Triggers ET_EXPECT_DEATH if log_intermediate_output_delegate has no data sink | |||
| static void expect_log_intermediate_delegate_death( | |||
There was a problem hiding this comment.
Love this helper function! Please make it as a protected member function of ProfilerETDumpTest to prohibit misuse. See https://github.com/pytorch/executorch/blob/main/kernels/test/op_split_copy_test.cpp#L94 for an example.
There was a problem hiding this comment.
Moved the helper function, I appreciate the example!
|
|
||
| auto buffer_data_sink = BufferDataSink::create(ptr, 2048); | ||
| auto buffer_data_sink = BufferDataSink::create(ptr, debug_buf_size); | ||
| auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); | ||
|
|
||
| etdump_gen[i]->create_event_block("test_block"); |
|
Thanks for your great work! Merged! |
…termediateOutput (#9762) ### Summary Fixes #9551 Reset ETDumpGen instances in each test iteration to avoid shared state to correctly trigger ET_EXPECT_DEATH when no DataSink is set. ### Test plan buck2 run devtools/etdump/tests:etdump_test -- --------- Co-authored-by: Gasoonjia <[email protected]>
mergennachin
added
the
community: contribution
Summary
Fixes #9551
Reset ETDumpGen instances in each test iteration to avoid shared state to correctly trigger ET_EXPECT_DEATH when no DataSink is set.
Test plan
buck2 run devtools/etdump/tests:etdump_test --