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.

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

add onnx-subgraph full changes code to Samsung ONE/tools #14613

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

chenyx113
Copy link

PR with full changes of onnx-subgraph tool.
related issue of: #14534
historical draft PR:
#14593
#14563

we 'd like our code to be an individual tool in ONE/tools path.
./nnas format & ./nnfw copyright-check have been run successfully locally.

please help review, any suggestions and comments will be appreciated, thank you!

ONE-DCO-1.0-Signed-off-by: Youxin Chen [email protected]

@seanshpark
Copy link
Contributor

I get

fatal: Need to specify how to reconcile divergent branches.

error when I pull this branch. Maybe rebase to master may fix this problem?

git clone https://github.com/ekg/glia.git
cp -r glia/json ../include
cp glia/json-forwards.h ../include
cp glia/jsoncpp.cpp ../src/lib
Copy link
Contributor

Choose a reason for hiding this comment

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

copy files will be listed as unchanged files in git status.
plz do not copy files like this.

I cannot find where jsoncpp.cpp file is used.

Comment on lines 13 to 14


Copy link
Contributor

@seanshpark seanshpark Feb 6, 2025

Choose a reason for hiding this comment

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

Suggested change

and also to all other codes that have unnecessary double lines

target_link_libraries(onnx-subgraph onnx-subgraph-parser ${Python3_LIBRARIES})


set(ONNX_SUGRAPH_FILES
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set(ONNX_SUGRAPH_FILES
set(ONNX_SUGRAPH_FILES

## Pre-steps
### Download the test AI models
1. bash scripts/test_model_download.sh, then "resnet-test.onnx" will be got in ./build
2. you can change to any other onnx files as your needs, or edit the download link in "scripts/test_model_download.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

if this line exceeds 100 cols, please split so that will be within 100 cols.
plz apply to all other codes

Comment on lines 26 to 27
#include "json.h"
enum class DeviceType
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include "json.h"
enum class DeviceType
#include "json.h"
enum class DeviceType

please add some empty lines between codes, like here and between methods/functions
so that reading is better.
too tight codes are hard to read.
plz apply to all other codes.

@seanshpark
Copy link
Contributor

I've left some comments with rought review with formats.
Plz post small PRs as I explained before.

@chenyx113
Copy link
Author

I've left some comments with rought review with formats. Plz post small PRs as I explained before.

ok, I will split it to small PRs, thanks for your review

@chenyx113
Copy link
Author

close this PR, and split to small PRs

@chenyx113 chenyx113 closed this Feb 6, 2025
@seanshpark
Copy link
Contributor

@chenyx113 , it would be better to leave this PR open.
I need to see this draft PR to understand the PR, how the changes are used, called, and so on,
to understand better of your codes.

@seanshpark
Copy link
Contributor

When the initial CMakeLists.txt file is landed, I'll enable a Workflow to check build and test.
So please post PR with this file first.

@chenyx113 chenyx113 reopened this Feb 7, 2025
@chenyx113
Copy link
Author

When the initial CMakeLists.txt file is landed, I'll enable a Workflow to check build and test. So please post PR with this file first.

ok,I will prepare as suggestion, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants