JSON format for Chakra ET #145

rvinaybharadwaj · 2024-08-02T15:52:21Z

Summary

Added JSON format for Chakra ET
To maintain backward compatibility, a wrapper is included that presents a common API for Protobuf and JSON

Test Plan

Added wrapperTests.cpp and test data

github-actions · 2024-08-02T15:52:33Z

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

JoongunPark · 2024-09-03T17:23:03Z

Hi @rvinaybharadwaj,

Thank you for the PR! I have a few requests:

Could you ensure that the code passes all checks (file formatting, tests, etc.)?
Could you provide step-by-step commands to help us reproduce the results?
What is the expected outcome, and how can we verify it? (Could you share us example input and output?)

Thanks again!

rvinaybharadwaj · 2024-09-05T18:36:17Z

Hi @rvinaybharadwaj,

Thank you for the PR! I have a few requests:

Could you ensure that the code passes all checks (file formatting, tests, etc.)?

Could you provide step-by-step commands to help us reproduce the results?

What is the expected outcome, and how can we verify it? (Could you share us example input and output?)

Thanks again!

Hi @JoongunPark,

I have run clang-format on the code and the tests are breaking on not finding a module during the setup phase. So, I don't think it's an issue related to my code. And I am not sure why the CLA check is failing even though I am a member of MLCommons.
I have added wrapper tests, which are functionally equivalent to the feeder tests and have included test data. You can run it through the existing google test framework.
The data in chakra.0.json is the same as the data in chakra.0.et You should be able to achieve the same result as the protobuf ET and the wrapper tests should all pass.

Let me know if you have any more questions.

JoongunPark · 2024-09-06T22:21:03Z

Hi @rvinaybharadwaj,

Thank you for the update!

It looks like this branch has new conflicts after the recent merge. Could you please resolve them?
For the cpp_linter, you might want to update the GitHub workflow to fix it. Here’s the link: cpp_lint.yml.
Regarding the CLA issue, I believe someone from MLCommons needs to add you. @tushar-krishna or @srinivas212 should be able to help with this.

I'll run some tests and share the results.

rvinaybharadwaj · 2024-09-06T23:05:09Z

Hi @rvinaybharadwaj,

Thank you for the update!

It looks like this branch has new conflicts after the recent merge. Could you please resolve them?

For the cpp_linter, you might want to update the GitHub workflow to fix it. Here’s the link: cpp_lint.yml.

Regarding the CLA issue, I believe someone from MLCommons needs to add you. @tushar-krishna or @srinivas212 should be able to help with this.

I'll run some tests and share the results.

Hi @JoongunPark ,
Thanks for the feedback. Merge conflicts have been fixed.

JoongunPark · 2024-09-15T19:30:04Z

Hi @rvinaybharadwaj,

Thank you for updating your PR!
I have two last requests:

Could you please ensure that the "CLA" test passes? It's mandatory, and unfortunately, I can't assist with this part.
It seems you've successfully addressed the original issue with the C++ Link (the module installation problem). However, there are still a few minor errors. You can check them here: link. Could you please take a look and fix those as well?

Thank you for your efforts!

rvinaybharadwaj · 2024-09-19T16:05:27Z

Hi @rvinaybharadwaj,

Thank you for updating your PR! I have two last requests:

Could you please ensure that the "CLA" test passes? It's mandatory, and unfortunately, I can't assist with this part.

It seems you've successfully addressed the original issue with the C++ Link (the module installation problem). However, there are still a few minor errors. You can check them here: link. Could you please take a look and fix those as well?

Thank you for your efforts!

Hi @JoongunPark,
All errors have been fixed.

JoongunPark · 2024-09-24T19:13:13Z

@rvinaybharadwaj
Thank you for resolving all the issues! I've started reviewing your code in detail, and here’s my understanding so far:

1. New Classes

JSONNode: Handles execution trace (ET) parsing from JSON files.
WrapperNode: Abstracts ET handling for multiple formats (e.g., .et and .json), working within the ETFeeder component.

2. Dependency Changes

pyproject.toml: Updated to support protobuf==5.*. This might resolve some installation conflicts with the original version.

3. Test Coverage

Unit Tests (tests/feeder/wrapperTests.cpp): Verifies the JSON parsing functionality.
Test Data (tests/data): Includes new JSON-format test files.

4. Linting Updates

clang-format-lint: Clang format (11->16). Linter version (v0.11 -> v0.18.1)

5. New Third-Party Library

json.hpp (src/third_party/utils/json.hpp): Utilized by JSONNode for JSON parsing.

Also, I have a question regarding JSON format trace. This PR does not include trace conversion from ET to JSON.
Is this PR assuming to get JSON trace with exsting Jsonizer?

Thank you again!

rvinaybharadwaj · 2024-09-24T19:58:57Z

@rvinaybharadwaj Thank you for resolving all the issues! I've started reviewing your code in detail, and here’s my understanding so far:

1. New Classes

JSONNode: Handles execution trace (ET) parsing from JSON files.

WrapperNode: Abstracts ET handling for multiple formats (e.g., .et and .json), working within the ETFeeder component.

2. Dependency Changes

pyproject.toml: Updated to support protobuf==5.*. This might resolve some installation conflicts with the original version.

3. Test Coverage

Unit Tests (tests/feeder/wrapperTests.cpp): Verifies the JSON parsing functionality.

Test Data (tests/data): Includes new JSON-format test files.

4. Linting Updates

clang-format-lint: Clang format (11->16). Linter version (v0.11 -> v0.18.1)

5. New Third-Party Library

json.hpp (src/third_party/utils/json.hpp): Utilized by JSONNode for JSON parsing.

Also, I have a question regarding JSON format trace. This PR does not include trace conversion from ET to JSON. Is this PR assuming to get JSON trace with exsting Jsonizer?

Thank you again!

@JoongunPark
Thank you for reviewing the code and summarizing. Your understanding of different parts of the code is correct. The existing JSONizer outputs a slightly different format, but I was assuming it is easy to do a JSON dump directly from Pytorch or any other tool.

JoongunPark · 2024-09-24T21:14:59Z

@rvinaybharadwaj Thank you for resolving all the issues! I've started reviewing your code in detail, and here’s my understanding so far:

1. New Classes

JSONNode: Handles execution trace (ET) parsing from JSON files.

WrapperNode: Abstracts ET handling for multiple formats (e.g., .et and .json), working within the ETFeeder component.

2. Dependency Changes

pyproject.toml: Updated to support protobuf==5.*. This might resolve some installation conflicts with the original version.

3. Test Coverage

Unit Tests (tests/feeder/wrapperTests.cpp): Verifies the JSON parsing functionality.

Test Data (tests/data): Includes new JSON-format test files.

4. Linting Updates

clang-format-lint: Clang format (11->16). Linter version (v0.11 -> v0.18.1)

5. New Third-Party Library

json.hpp (src/third_party/utils/json.hpp): Utilized by JSONNode for JSON parsing.

Also, I have a question regarding JSON format trace. This PR does not include trace conversion from ET to JSON. Is this PR assuming to get JSON trace with exsting Jsonizer?
Thank you again!

@JoongunPark Thank you for reviewing the code and summarizing. Your understanding of different parts of the code is correct. The existing JSONizer outputs a slightly different format, but I was assuming it is easy to do a JSON dump directly from Pytorch or any other tool.

Regarding the JSON format trace, I believe we should discuss whether to align it with the existing JSONizer or consider using a different format. This decision might be crucial, as I recall your initial plan was to create a merged, unified JSON trace. We can also involve @TaekyungHeo for further discussion on this matter.

TaekyungHeo

I left several comments and will leave more when I have more time.

.github/workflows/cpp_lint.yml

pyproject.toml

src/feeder/JSONNode.cpp

src/feeder/JSONNode.h

src/feeder/WrapperNode.cpp

src/feeder/et_feeder.cpp

tests/feeder/wrapperTests.cpp

src/feeder/JSONNode.cpp

tests/data/jsonData.tar.gz

tests/feeder/tests.cpp

tests/feeder/wrapperTests.cpp

tests/data/jsonData.tar.gz

src/feeder/et_feeder.h

fix lint errors fix lint errors fix lint errors

rvinaybharadwaj requested a review from a team as a code owner August 2, 2024 15:52

willjwon requested review from JoongunPark and willjwon August 13, 2024 01:00

rvinaybharadwaj force-pushed the jsonify branch 2 times, most recently from ae9ec40 to 333a9a3 Compare September 19, 2024 16:03

rvinaybharadwaj force-pushed the jsonify branch from 333a9a3 to d7da765 Compare September 23, 2024 16:58

TaekyungHeo reviewed Sep 25, 2024

View reviewed changes

tests/data/jsonData.tar.gz Outdated Show resolved Hide resolved

TaekyungHeo reviewed Sep 25, 2024

View reviewed changes

src/feeder/et_feeder.h Show resolved Hide resolved

TaekyungHeo mentioned this pull request Oct 7, 2024

Fix CI issues #157

Merged

rvinaybharadwaj added 9 commits October 7, 2024 17:44

Add JSON support + wrapper

1559bcf

Rebasing with main

649c37b

adding json test data

8e311bc

adding wrapper tests

6054b0b

code cleanup: make class members private

f93765f

Updating tests

8351938

changing datatypes to match protobuf

9796253

adding missed datatype changes

0ce0ae2

updating WrapperTests datatypes

67d29cd

rvinaybharadwaj added 18 commits October 7, 2024 17:44

removing involved dims and minor bug fix

045bd6f

fixing include path in et_feeder

e5a4627

add missing break statement

46d42d6

minor bug fixes

69dffb1

fix include path

3b7a0ad

fix lint errors

6cbbc4f

merging et_feeder_node

124de08

adding install setuptools to github workflows

274826f

updating workflows

21417d8

updating workflows

ad998fc

updating cpp_lint.yml

bf61b64

updating clang-format version

2474103

updating clang-format version

3a7c2c9

fix lint errors

7756ec2

fix lint errors fix lint errors fix lint errors

Fix rebase error

f439bf4

addressing reviewer comments

58fda3e

fix lint errors

61c74d8

fix lint errors

f803f33

rvinaybharadwaj force-pushed the jsonify branch from cc4ceba to f803f33 Compare October 7, 2024 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON format for Chakra ET #145

JSON format for Chakra ET #145

rvinaybharadwaj commented Aug 2, 2024

github-actions bot commented Aug 2, 2024 •

edited

Loading

JoongunPark commented Sep 3, 2024 •

edited

Loading

rvinaybharadwaj commented Sep 5, 2024

JoongunPark commented Sep 6, 2024

rvinaybharadwaj commented Sep 6, 2024 •

edited

Loading

JoongunPark commented Sep 15, 2024

rvinaybharadwaj commented Sep 19, 2024

JoongunPark commented Sep 24, 2024 •

edited

Loading

rvinaybharadwaj commented Sep 24, 2024 •

edited

Loading

1. New Classes

2. Dependency Changes

3. Test Coverage

4. Linting Updates

5. New Third-Party Library

JoongunPark commented Sep 24, 2024

1. New Classes

2. Dependency Changes

3. Test Coverage

4. Linting Updates

5. New Third-Party Library

TaekyungHeo left a comment

JSON format for Chakra ET #145

Are you sure you want to change the base?

JSON format for Chakra ET #145

Conversation

rvinaybharadwaj commented Aug 2, 2024

Summary

Test Plan

github-actions bot commented Aug 2, 2024 • edited Loading

JoongunPark commented Sep 3, 2024 • edited Loading

rvinaybharadwaj commented Sep 5, 2024

JoongunPark commented Sep 6, 2024

rvinaybharadwaj commented Sep 6, 2024 • edited Loading

JoongunPark commented Sep 15, 2024

rvinaybharadwaj commented Sep 19, 2024

JoongunPark commented Sep 24, 2024 • edited Loading

1. New Classes

2. Dependency Changes

3. Test Coverage

4. Linting Updates

5. New Third-Party Library

rvinaybharadwaj commented Sep 24, 2024 • edited Loading

1. New Classes

2. Dependency Changes

3. Test Coverage

4. Linting Updates

5. New Third-Party Library

JoongunPark commented Sep 24, 2024

1. New Classes

2. Dependency Changes

3. Test Coverage

4. Linting Updates

5. New Third-Party Library

TaekyungHeo left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 2, 2024 •

edited

Loading

JoongunPark commented Sep 3, 2024 •

edited

Loading

rvinaybharadwaj commented Sep 6, 2024 •

edited

Loading

JoongunPark commented Sep 24, 2024 •

edited

Loading

rvinaybharadwaj commented Sep 24, 2024 •

edited

Loading