Skip to content

[ml service] Add Flare as a new nnfw type - #638

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:dev_support_flare
Open

[ml service] Add Flare as a new nnfw type#638
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:dev_support_flare

Conversation

@songgot

@songgot songgot commented Jul 16, 2025

Copy link
Copy Markdown
Contributor
  • Implemented support for 'flare' nnfw type in ML service API
  • Included test cases to validate flare functionality

@songgot
songgot force-pushed the dev_support_flare branch 2 times, most recently from 30bee27 to 3f7800a Compare July 16, 2025 08:35
@myungjoo

Copy link
Copy Markdown
Member

Remove unrelated commits. Make it possible to be reviewed and merged independently from other topics

@songgot
songgot force-pushed the dev_support_flare branch 3 times, most recently from 3dbf288 to 027d334 Compare July 29, 2025 08:23
@songgot

songgot commented Jul 29, 2025

Copy link
Copy Markdown
Contributor Author

Remove unrelated commits. Make it possible to be reviewed and merged independently from other topics

Thank you for taking the time to review.
I've updated.

@songgot
songgot force-pushed the dev_support_flare branch 3 times, most recently from 5eb5c42 to 21b0002 Compare July 30, 2025 00:10
Comment thread c/src/ml-api-inference-single.c Outdated
@songgot
songgot force-pushed the dev_support_flare branch 4 times, most recently from 65bad43 to 2515ccc Compare July 31, 2025 07:15
Comment thread tests/capi/unittest_capi_service_extension.cc Outdated
Comment thread tests/capi/unittest_capi_service_extension.cc Outdated
@songgot
songgot force-pushed the dev_support_flare branch 4 times, most recently from bbda0b6 to 13ee0d7 Compare August 26, 2025 04:54
Comment thread c/include/nnstreamer-tizen-internal.h Outdated
- 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>

@hj210 hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

Comment thread c/src/ml-api-inference-single.c
Comment thread tests/capi/unittest_capi_service_extension.cc
Comment thread tests/capi/unittest_capi_service_extension.cc
Comment thread tests/capi/unittest_capi_service_extension.cc

@myungjoo-bot myungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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).

  1. [High] Merge conflict / stale basegh reports CONFLICTING; git merge-tree conflicts in c/src/ml-api-inference-single.c (PR ~2083-2097) and tests/capi/unittest_capi_service_extension.cc (PR ~394-541). main has since introduced _is_valid_extension() and switch ((int) *nnfw) (2a038f8), and replaced the llama.cpp test macro with LLAMACPP_TEST_MODEL / skip_llamacpp_tc (acb6d92). Since main's switch ((int) *nnfw) already removes the -Werror=switch obstacle, the FLARE check should now be a normal case 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.
  2. [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 is strcasecmp at c/src/ml-api-inference-single.c:1065 — use g_ascii_strcasecmp like the rest of the file. Please rebase and get CI green.
  3. [Medium] Flare-specific hack instead of generic resolutionml-api-inference-single.c:1062-1067 hard-codes "flare" before validation. _ml_get_nnfw_type_by_subplugin_name ("flare") already returns 23, so the generic fix is if (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.
  4. [Medium] Temporary API placementc/include/nnstreamer-tizen-internal.h:22: a bare #define shadows the ml_nnfw_type_e namespace 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 into ml_nnfw_type_e in ml-api-common.h with ACR. Please do that in this PR (public API doc change included), or document who/when removes the define.
  5. [Medium] Tests never run in CI and are mis-gatedscenarioConfigFlare is inside #if defined(ENABLE_LLAMACPP), and there is no Flare subplugin in CI, so the new validation/open path has zero coverage. skip_llm_tc only checks sflare_if_4bit_3b.bin but the config also needs history_lora.bin and data/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_file with ML_NNFW_TYPE_FLARE rejecting non-.bin files with ML_ERROR_INVALID_PARAMETER; gate the scenario test on its own condition and check all three asset files.
  6. [Low] Malformed test prompttests/test_models/data/flare_input.txt contains literal two-character \n sequences, a stray trailing ", and no trailing newline.
  7. [Low] Styleml-api-inference-single.c:2097 @todo comment is indented ~40 spaces; unittest_capi_service_extension.cc adds <iostream> / std::cout.write where the file uses g_print (use g_print ("%.*s", ...)); gchar *input_file should be const gchar *; the g_strdup ("flare_input.txt") is unnecessary; stray blank lines at ~409/474/542; the 40 s blocking g_usleep should be a bounded poll on tdata->received.

No back-door or suspicious behavior found.

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.

5 participants