[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
3f7800a
Compare
|
Remove unrelated commits. Make it possible to be reviewed and merged independently from other topics |
3dbf288 to
027d334
Compare
Thank you for taking the time to review. |
5eb5c42 to
21b0002
Compare
65bad43 to
2515ccc
Compare
bbda0b6 to
13ee0d7
Compare
- 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
7330e84
Compare
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:
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.