Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Missing FileDataSink test in etdump_flatcc tests #9165

Closed
Gasoonjia opened this issue Mar 11, 2025 · 4 comments · Fixed by #9332
Closed

Missing FileDataSink test in etdump_flatcc tests #9165

Gasoonjia opened this issue Mar 11, 2025 · 4 comments · Fixed by #9332
Assignees
@Megan0704-1
Labels
good first issue Good for newcomers module: devtools Issues related to developer tools and code under devtools/

Comments

@Gasoonjia
Copy link
Contributor

Gasoonjia commented Mar 11, 2025 •
edited by pytorch-bot bot
Loading

🚀 The feature, motivation and pitch

We have adopted datasink as the primary output data dumping pipeline in ETDumpGen, and we've implemented two datasinks, BufferDataSink and FileDataSink. #9033 has more details.
However, we only tested BufferDataSink in the ETDumpGen's test (e.g. https://github.com/pytorch/executorch/blob/main/devtools/etdump/tests/etdump_test.cpp#L237) but still missing FileDataSink test.
So, for this issue we'd love to bring FileDataSink into ETDumpGen's test.
You can mimic what we've done for BufferDataSink to solve the problem, and https://github.com/pytorch/executorch/blob/main/extension/data_loader/test/file_data_loader_test.cpp would be a good example about how to create a temporary file for testing.

Alternatives

No response

Additional context

No response

RFC (Optional)

No response

cc @Jack-Khuu
@Gasoonjia Gasoonjia added good first issue Good for newcomers module: devtools Issues related to developer tools and code under devtools/ labels Mar 11, 2025
@Megan0704-1
Copy link
Contributor

Hello!
This is my first time contributing to this repository, and I’d love to work on this issue.
Could you assign it to me?
I plan to refer to the example and follow the BufferDataSink test structure to implement the solution.

@Gasoonjia
Copy link
Contributor Author

Gasoonjia commented Mar 14, 2025 •
edited
Loading

yes of course! Thanks for your contribution! @Megan0704-1
Please let me know if anything i can help, or reach out to me via our discord!

Megan0704-1 pushed a commit to Megan0704-1/executorch that referenced this issue Mar 17, 2025
test(etdump): Add FileDataSink test coverage
164489b 
Summary:
- Extend test loops to cover FileDataSink scenarios (j=2 case)
- Add temporary file cleanup in TearDown
- Include file_data_sink.h, stdio.h and fstream in test sources
- Update CMakeLists.txt to ensure proper linking

Fixes pytorch#9165
@Megan0704-1 Megan0704-1 mentioned this issue Mar 17, 2025
Merged
@Megan0704-1
Copy link
Contributor

Hi @Gasoonjia
I’ve addressed it in PR #9332, which adds FileDataSink validation to the etdump_test.cpp debug event tests.
The changes include:

  • Extending test loops to cover all 3 data sink types (span/buffer/file)
  • Adding temporary file creation/cleanup logic
  • Ensuring CMake linkage for FileDataSink
    Could you please review the PR when you have time?
    Let me know if there’s anything I missed or if additional test cases would be helpful.

Thanks for the clear issue description—it made the fix straightforward! 🙌

@Gasoonjia
Copy link
Contributor Author

Thanks @Megan0704-1 for your contribution! Have left some comments under your PR.

Megan0704-1 pushed a commit to Megan0704-1/executorch that referenced this issue Mar 18, 2025
@Megan0704-1
test(etdump): Add FileDataSink test coverage
b1263d9 
Summary:
- Extend test loops to cover FileDataSink scenarios (j=2 case)
- Add temporary file cleanup in TearDown
- Include file_data_sink.h, stdio.h and fstream in test sources
- Update CMakeLists.txt to ensure proper linking

Fixes pytorch#9165
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Megan0704-1 Megan0704-1

Labels
good first issue Good for newcomers module: devtools Issues related to developer tools and code under devtools/
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test(etdump): Add FileDataSink test coverage Megan0704-1/executorch
2 participants
@Gasoonjia @Megan0704-1