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

[luci/import] Revise ImportEx #13850

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

seanshpark
Copy link
Contributor

This will revise ImportEx ctro with graph builder source and new importModule method.

@seanshpark
Copy link
Contributor Author

seanshpark commented Aug 30, 2024

this change is necessary to make embedded-import-value-test module work.
embedded-import-value-test is the only client that will use this methods.

@seanshpark seanshpark requested a review from a team August 30, 2024 00:47
shs-park
shs-park previously approved these changes Aug 30, 2024
Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

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

LGTM!
=)

@seanshpark
Copy link
Contributor Author

Um?

[2024-08-30T00:55:52.100Z] [  489s] [ 68%] Built target onert_backend_cl_common
[2024-08-30T00:55:52.100Z] [  490s] [ 68%] Building CXX object runtime/onert/odc/CMakeFiles/onert_odc.dir/Embedder.cc.o
[2024-08-30T00:55:52.357Z] [  490s] In file included from /home/abuild/rpmbuild/BUILD/nnfw-1.29.0/build/overlay/include/luci/Import/GraphBuilderContext.h:20,
[2024-08-30T00:55:52.357Z] [  490s]                  from /home/abuild/rpmbuild/BUILD/nnfw-1.29.0/build/overlay/include/luci/Import/GraphBuilderBase.h:20,
[2024-08-30T00:55:52.357Z] [  490s]                  from /home/abuild/rpmbuild/BUILD/nnfw-1.29.0/build/overlay/include/luci/Import/GraphBuilderRegistry.h:20,
[2024-08-30T00:55:52.357Z] [  490s]                  from /home/abuild/rpmbuild/BUILD/nnfw-1.29.0/build/overlay/include/luci/ImporterEx.h:22,
[2024-08-30T00:55:52.357Z] [  490s]                  from /home/abuild/rpmbuild/BUILD/nnfw-1.29.0/runtime/onert/odc/Embedder.cc:22:
[2024-08-30T00:55:52.357Z] [  490s] /home/abuild/rpmbuild/BUILD/nnfw-1.29.0/build/overlay/include/luci/Import/CircleReader.h:20:10: fatal error: mio/circle/schema_generated.h: No such file or directory
[2024-08-30T00:55:52.357Z] [  490s]    20 | #include <mio/circle/schema_generated.h>
[2024-08-30T00:55:52.357Z] [  490s]       |          ^~~~~~~~~~~~~~~

@seanshpark
Copy link
Contributor Author

@hseok-oh do you know why this happens and how to fix this?

@seanshpark
Copy link
Contributor Author

@hseok-oh , #include <luci/ImporterEx.h> in Embedder.cc seems unnecessary header.
Can you check it?

This will revise ImportEx ctro with graph builder source and new importModule method.

ONE-DCO-1.0-Signed-off-by: SaeHie Park <saehie.park@gmail.com>
@seanshpark
Copy link
Contributor Author

seanshpark commented Aug 30, 2024

@shs-park , PTAL again,
I've modified a little bit to avoid tizen-gbs build failure.
I'll update this change after onert Embedder.cc removes include to ImporterEx.h.

Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

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

👍

@seanshpark seanshpark merged commit 7aee01c into Samsung:master Aug 30, 2024
10 checks passed
@seanshpark seanshpark deleted the luci_imp_impex branch August 30, 2024 02:45
@hseok-oh
Copy link
Contributor

hseok-oh commented Aug 30, 2024

Embedder.cc is not using ImporterEx.h, but runtime/onert/odc/Quantizer.cc is using ImporterEx.h to export quantized circle file. So this issue still remains.

In fact, runtime is already using workaround to avoid build fail by referencing not installed header from luci public header.

ONE/Makefile.template

Lines 170 to 172 in 7aee01c

@cp compiler/angkor/include/nncc/core/ADT/tensor/Index.h ${OVERLAY_FOLDER}/include/nncc/core/ADT/tensor
@cp compiler/oops/include/oops/InternalExn.h ${OVERLAY_FOLDER}/include/oops
@cp compiler/luci/lang/include/luci/IR/CircleNodes.lst ${OVERLAY_FOLDER}/include/luci/IR

ONE/packaging/nnfw.spec

Lines 205 to 207 in 7aee01c

cp compiler/angkor/include/nncc/core/ADT/tensor/Index.h %{overlay_path}/include/nncc/core/ADT/tensor
cp compiler/oops/include/oops/InternalExn.h %{overlay_path}/include/oops
cp compiler/luci/lang/include/luci/IR/CircleNodes.lst %{overlay_path}/include/luci/IR

Maybe we can avoid this issue by using same workaround.
Anyway, I inform that luci public headers have some dependency issue. It may be passed if we build compiler modules at once, but developer may meet same issue with this when developer tries to include installed luci headers.

@seanshpark
Copy link
Contributor Author

To use luci, onert must include dependent modules for building.
Just copy header files that looks like using doesn't seem to be a good way.

@hseok-oh
Copy link
Contributor

hseok-oh commented Aug 30, 2024

To use luci, onert must include dependent modules for building. Just copy header files that looks like using doesn't seem to be a good way.

Including luci and dependent module build in runtime cmake script is not easy, so I divided build process on makefile (and tizen spec file) level for luci and runtime. So runtime uses installed compiler libraries and headers.

On the other hand, areser is included in runtime cmake script because arser is header-only.

# Add arser for test driver
# TODO: Better way to handle this
if(ENABLE_TEST)
add_library(arser INTERFACE)
target_include_directories(arser INTERFACE ${NNAS_PROJECT_SOURCE_DIR}/compiler/arser/include/)
endif(ENABLE_TEST)

@seanshpark

This comment was marked as duplicate.

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.

3 participants