Uh oh!
There was an error while loading. Please reload this page.
[ml service] Add Flare as a new nnfw type - #638
Conversation
songgot
commented
Jul 16, 2025
- Implemented support for 'flare' nnfw type in ML service API
- Included test cases to validate flare functionality
30bee27 to
3f7800aComparemyungjoo
commented
Jul 24, 2025
Remove unrelated commits. Make it possible to be reviewed and merged independently from other topics |
3dbf288 to
027d334Comparesonggot
commented
Jul 29, 2025
Thank you for taking the time to review. |
5eb5c42 to
21b0002CompareUh oh!
There was an error while loading. Please reload this page.
65bad43 to
2515cccCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bbda0b6 to
13ee0d7CompareUh oh!
There was an error while loading. Please reload this page.
- Implemented support for 'flare' nnfw type in ML service API - Included test cases to validate flare functionality Signed-off-by: hyunil park <hyunil46.park@samsung.com>
13ee0d7 to
7330e84Compare
hj210
left a comment
There was a problem hiding this comment.
The commit purpose and changes are clearly described, and the test code appears to be well-written. Here are a few suggestions to improve maintainability and code quality:
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
myungjoo-bot
left a comment
There was a problem hiding this comment.
Automated review (transcribed from an AI review agent's report; please verify before acting).
Summary: The PR adds a temporary Tizen-internal #define ML_NNFW_TYPE_FLARE 23, registers "flare" in ml_nnfw_subplugin_name[], special-cases fw_name == "flare" in ml_single_open_custom(), adds a pre-switch .bin extension check in _ml_validate_model_file(), and generalizes the llama.cpp ml-service scenario test. I checked the diff against current main, merge-ability, CI status, and whether main already has an equivalent type (it does not: no FLARE / QUICKAI in main).
- [High] Merge conflict / stale base —
ghreportsCONFLICTING;git merge-treeconflicts inc/src/ml-api-inference-single.c(PR ~2083-2097) andtests/capi/unittest_capi_service_extension.cc(PR ~394-541).mainhas since introduced_is_valid_extension()andswitch ((int) *nnfw)(2a038f8), and replaced the llama.cpp test macro withLLAMACPP_TEST_MODEL/skip_llamacpp_tc(acb6d92). Sincemain'sswitch ((int) *nnfw)already removes the-Werror=switchobstacle, the FLARE check should now be a normalcase ML_NNFW_TYPE_FLARE:using_is_valid_extension (model[0], { ".bin", NULL }), as @jaeyun-jung requested, instead of the pre-switch block and its@todo. - [High] Android CI is red for this PR only — all four
build (arm64-v8a / armeabi-v7a / x86 / x86_64)jobs failed while neighbouring PRs passed. Logs have expired so I could not confirm the cause; the prime suspect isstrcasecmpatc/src/ml-api-inference-single.c:1065— useg_ascii_strcasecmplike the rest of the file. Please rebase and get CI green. - [Medium] Flare-specific hack instead of generic resolution —
ml-api-inference-single.c:1062-1067hard-codes"flare"before validation._ml_get_nnfw_type_by_subplugin_name ("flare")already returns 23, so the generic fix isif (nnfw == ML_NNFW_TYPE_ANY && info->fw_name) nnfw = _ml_get_nnfw_type_by_subplugin_name (info->fw_name);, which also helps any other framework whose model extension is not auto-detectable. - [Medium] Temporary API placement —
c/include/nnstreamer-tizen-internal.h:22: a bare#defineshadows theml_nnfw_type_enamespace and has no(Since X.0)Doxygen; the stated migration condition (after Tizen 10.0 M2) has passed and @myungjoo already asked for this to be moved intoml_nnfw_type_einml-api-common.hwith ACR. Please do that in this PR (public API doc change included), or document who/when removes the define. - [Medium] Tests never run in CI and are mis-gated —
scenarioConfigFlareis inside#if defined(ENABLE_LLAMACPP), and there is no Flare subplugin in CI, so the new validation/open path has zero coverage.skip_llm_tconly checkssflare_if_4bit_3b.binbut the config also needshistory_lora.binanddata/tokenizer.json(partial assets fail instead of skip), and its skip message still points to the Llama-2 URL. Please add model-free unit tests that do run in CI:_ml_get_nnfw_type_by_subplugin_name ("flare") == ML_NNFW_TYPE_FLARE, its reverse, and_ml_validate_model_filewithML_NNFW_TYPE_FLARErejecting non-.binfiles withML_ERROR_INVALID_PARAMETER; gate the scenario test on its own condition and check all three asset files. - [Low] Malformed test prompt —
tests/test_models/data/flare_input.txtcontains literal two-character\nsequences, a stray trailing", and no trailing newline. - [Low] Style —
ml-api-inference-single.c:2097@todocomment is indented ~40 spaces;unittest_capi_service_extension.ccadds<iostream>/std::cout.writewhere the file usesg_print(useg_print ("%.*s", ...));gchar *input_fileshould beconst gchar *; theg_strdup ("flare_input.txt")is unnecessary; stray blank lines at ~409/474/542; the 40 s blockingg_usleepshould be a bounded poll ontdata->received.
No back-door or suspicious behavior found.