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

[Application] Bug fix about meson setting @open sesame 06/10 13:02 #2627

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

baek2sm
Copy link
Contributor

@baek2sm baek2sm commented Jun 7, 2024

Now, PICO GPT and LLAMA are adding extra_defines meson option in the application side.

However, even if this code is executed during build, this definition is not reflected when actually running the app.

Because the application area is built after the process of reflecting extra_defines to add_project_arguments has already been completed, so adding extra_defines during application build is meaningless.

In addition, it is impossible to call add_project_arguments after build, so the structure to add extra_defines during build process is wrong.

The reason why PICO GPT and LLAMA add extra_defines is that the JSON-related script created now does not run on tizen, so json-related option was added to the root meson and the options on the application side were removed.

Self evaluation:

  1. Build test: [X]Passed [ ]Failed [ ]Skipped
  2. Run test: [X]Passed [ ]Failed [ ]Skipped

@taos-ci
Copy link
Collaborator

taos-ci commented Jun 7, 2024

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2627. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

@taos-ci
Copy link
Collaborator

taos-ci commented Jun 7, 2024

:octocat: cibot: @baek2sm, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2627-202406071809100.8458559513092-85cc5e3160ee72c0ab0bf4fb87950237a52136bb/.

Copy link
Member

@myungjoo myungjoo left a comment

Choose a reason for hiding this comment

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

LGTM

@baek2sm Please let the guys working on LLM-on-Tizen (sitting next to you!) know this issue and let them check if this is ok or they need to port/fix something in Tizen. If they need to fix something in Tizen, they don't need to do it themselves; they may ask Tizen developers to fix them in case it's the issue in a Tizen package.

@baek2sm
Copy link
Contributor Author

baek2sm commented Jun 10, 2024

LGTM

@baek2sm Please let the guys working on LLM-on-Tizen (sitting next to you!) know this issue and let them check if this is ok or they need to port/fix something in Tizen. If they need to fix something in Tizen, they don't need to do it themselves; they may ask Tizen developers to fix them in case it's the issue in a Tizen package.

@myungjoo Okay, I will! Thanks :)

@baek2sm baek2sm changed the title [Application] Bug fix about meson setting [Application] Bug fix about meson setting @open sesame 06/10 13:02 Jun 10, 2024
@taos-ci
Copy link
Collaborator

taos-ci commented Jun 10, 2024

:octocat: cibot: @baek2sm, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2627-202406101303480.65484309196472-85cc5e3160ee72c0ab0bf4fb87950237a52136bb/.

meson.build Outdated
@@ -34,6 +34,8 @@ if get_option('platform') == 'tizen'
if get_option('enable-tizen-feature-check')
add_project_arguments('-D__FEATURE_CHECK_SUPPORT__', language: ['c', 'cpp'])
endif
else
add_project_arguments('-DENABLE_JSON=1', language: ['c', 'cpp'])
Copy link
Member

Choose a reason for hiding this comment

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

You prepared this for Android only? Then, add elif android here.

Ubuntu build:

[369/506] Compiling C++ object 'Applications/PicoGPT/jni/4c5dc94@@nntrainer_pico_gpt@exe/main.cpp.o'.
FAILED: Applications/PicoGPT/jni/4c5dc94@@nntrainer_pico_gpt@exe/main.cpp.o 
c++ -IApplications/PicoGPT/jni/4c5dc94@@nntrainer_pico_gpt@exe -IApplications/PicoGPT/jni -I../Applications/PicoGPT/jni -I../Applications/utils/jni/./includes -Inntrainer -I../nntrainer -Inntrainer/../api -I../nntrainer/../api -I../nntrainer/../api/ccapi/include -Inntrainer/compiler -I../nntrainer/compiler -Inntrainer/dataset -I../nntrainer/dataset -Inntrainer/layers/loss -I../nntrainer/layers/loss -Inntrainer/layers -I../nntrainer/layers -Inntrainer/models -I../nntrainer/models -Inntrainer/optimizers -I../nntrainer/optimizers -Inntrainer/tensor -I../nntrainer/tensor -Inntrainer/utils -I../nntrainer/utils -Inntrainer/graph -I../nntrainer/graph -I../api/ccapi/include -Iapi/ccapi/.. -I../api/ccapi/.. -I/usr/include/iniparser -I/usr/include/x86_64-linux-gnu/openblas-pthread/ -I/usr/include/nnstreamer -I/usr/include/tensorflow2/ -I/usr/lib/x86_64-linux-gnu/pkgconfig/../../include -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Werror -std=c++17 '-DVERSION="0.5.0"' -DVERSION_MAJOR=0 -DVERSION_MINOR=5 -DVERSION_MICRO=0 -DENABLE_JSON=1 -Wredundant-decls -Wwrite-strings -Wformat -Wformat-nonliteral -Wformat-security -Winit-self -Waddress -Wvla -Wpointer-arith -Wno-error=varargs -ftree-vectorize -Wno-maybe-uninitialized -Wno-unused-variable -DMIN_CPP_VERSION=201703L -DOPENCL_KERNEL_PATH=nntrainer_opencl_kernels -DML_API_COMMON=1 -DNNSTREAMER_AVAILABLE=1 -DUSE_BLAS=1 -DNNTR_NUM_THREADS=1 -DOMP_NUM_THREADS=1 -DDEBUG=1 -D__LOGGING__=1 -DENABLE_TEST=1 -DENABLE_NNSTREAMER_BACKBONE=1 -DENABLE_TFLITE_BACKBONE=1 -DENABLE_TFLITE_INTERPRETER=1 '-DNNTRAINER_CONF_PATH="/etc/nntrainer.ini"' -g -O2 -fdebug-prefix-map=/var/www/html/nntrainer/TAOS-CI/ci/repo-workers/pr-checker/2627-202406101303480.65484309196472-85cc5e3160ee72c0ab0bf4fb87950237a52136bb/nntrainer=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -DGTEST_HAS_PTHREAD=1 -Wl,--start-group -lpthread -Wl,--end-group -fopenmp -pthread -MD -MQ 'Applications/PicoGPT/jni/4c5dc94@@nntrainer_pico_gpt@exe/main.cpp.o' -MF 'Applications/PicoGPT/jni/4c5dc94@@nntrainer_pico_gpt@exe/main.cpp.o.d' -o 'Applications/PicoGPT/jni/4c5dc94@@nntrainer_pico_gpt@exe/main.cpp.o' -c ../Applications/PicoGPT/jni/main.cpp
../Applications/PicoGPT/jni/main.cpp:21:10: fatal error: encoder.hpp: No such file or directory
   21 | #include "encoder.hpp"
      |          ^~~~~~~~~~~~~
compilation terminated.

Copy link
Contributor Author

@baek2sm baek2sm Jun 11, 2024

Choose a reason for hiding this comment

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

@myungjoo (edited) Thanks, it seems like this code currently only works on Android well.

The reason why the CI passed for both Android and Ubuntu is before because, as mentioned in the PR message, the extra_defines related code has never worked properly before.

Although this code also works normally in my Ubuntu environment, I will only enable this option for Android until additional update measures are taken to make this code work in a general environment.

I will leave the issue about it.

Copy link
Contributor Author

@baek2sm baek2sm Jun 12, 2024

Choose a reason for hiding this comment

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

In #2632 , I left the issue of supporting encoders in Ubuntu and Tizen

Copy link
Member

@myungjoo myungjoo left a comment

Choose a reason for hiding this comment

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

Meson fix has an incorrect condition.

@baek2sm baek2sm added the bug Something isn't working label Jun 11, 2024
@baek2sm baek2sm removed the bug Something isn't working label Jun 11, 2024
@baek2sm baek2sm changed the title [Application] Bug fix about meson setting @open sesame 06/10 13:02 [WIP][Application] Bug fix about meson setting @open sesame 06/10 13:02 Jun 11, 2024
@taos-ci
Copy link
Collaborator

taos-ci commented Jun 11, 2024

:octocat: cibot: @baek2sm, The last line of a text file must have a newline character. Please append a new line at the end of the line in jni/prepare_encoder.sh.

Now, PICO GPT and LLAMA are adding extra_defines meson option in the application side.

However, even if this code is executed during build, this definition is not reflected when actually running the app.

Because the application area is built after the process of reflecting extra_defines to add_project_arguments has already been completed, so adding extra_defines during application build is meaningless.

In addition, it is impossible to call add_project_arguments after build, so the structure to add extra_defines during build process is wrong.

The reason why PICO GPT and LLAMA add extra_defines is that the encoder-related script created now does not run on tizen, so encoder-related option was added to the root meson and the options on the application side were removed.

**Self evaluation:**
1. Build test:     [X]Passed [ ]Failed [ ]Skipped
2. Run test:     [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Seungbaek Hong <[email protected]>
@baek2sm baek2sm changed the title [WIP][Application] Bug fix about meson setting @open sesame 06/10 13:02 [Application] Bug fix about meson setting @open sesame 06/10 13:02 Jun 11, 2024
Copy link
Collaborator

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

Copy link
Member

@SeoHyungjun SeoHyungjun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@skykongkong8 skykongkong8 left a comment

Choose a reason for hiding this comment

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

LGTM

@myungjoo myungjoo merged commit b99c609 into nnstreamer:main Jun 21, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants