[After #607][api/single] Support executorch-llama by anyj0527 · Pull Request #574 · nnstreamer/api

anyj0527 · 2024-10-29T08:12:17Z

Let single API handle flexible filter (executorch-llama)
Add a enum for new nnfw EXECUTORCH_LLAMA
Add a simple disabled test to show how to use executorch-llama with single api

taos-ci · 2024-10-29T08:12:20Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #574. 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

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

songgot · 2024-10-30T01:02:52Z

+  ml_tensors_info_set_tensor_type (in_info, 0, ML_TENSOR_TYPE_UINT8);
+  ml_tensors_info_set_tensor_dimension (in_info, 0, dim);
+
+  ml_tensors_data_create (in_info, &in_data);


How about checking if in_data is created properly?

status = ml_tensors_data_create (in_info, &in_data); EXPECT_EQ (status, ML_ERROR_NONE);

songgot · 2024-10-30T01:05:43Z

+  ml_tensors_info_set_tensor_dimension (in_info, 0, dim);
+
+  ml_tensors_data_create (in_info, &in_data);
+  ml_tensors_data_set_tensor_data (in_data, 0, prompt.c_str (), prompt.size () + 1);


Let's check the result, how about you?
EXPECT_EQ (status, ML_ERROR_NONE);

songgot

LGTM 👍

niley7464 · 2024-10-30T02:53:05Z

    goto error;
  }
+  /* handle flexible single */
+  if (info->nnfw == ML_NNFW_TYPE_EXECUTORCH_LLAMA) {


I think if there are more flexible filter, we may revise this part later.
The llama2c filter I created doesn't seem to be scalable due to API limitations, so I won't add it now.

niley7464

LGTM :)

myungjoo · 2024-10-30T08:04:11Z

  ML_NNFW_TYPE_NCNN = 18,             /**< Tencent ncnn (Since 9.0) */
  ML_NNFW_TYPE_TENSORRT = 19,         /**< NVidia Tensor-RT (Since 9.0) */
  ML_NNFW_TYPE_QNN = 20,              /**< Qualcomm QNN (Qualcomm® AI Engine Direct) (Since 9.0) */
+  ML_NNFW_TYPE_EXECUTORCH_LLAMA = 21, /**< ExecuTorch Llama runner */


Add Tizen version requirement

jaeyun-jung · 2024-11-20T08:18:23Z

Please do not merge this until tizen gbm release is done!

- Let single API handle flexible filter. Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

- Add a enum for new nnfw `EXECUTORCH_LLAMA` Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

- Let single API handle executorch-llama as flexible filter thus do invoke-dynamic. Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

- Add a simple test to show how to use executorch-llama with single api Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

myungjoo-bot

Automated review of this draft (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR (4 commits, +72 lines, all DCO-signed) lets the single API handle a flexible-format filter, adds ML_NNFW_TYPE_EXECUTORCH_LLAMA, and adds a disabled sample test. Commit 1 (1674c69) is byte-for-byte the content of #607, merged 2025-07-24, so it is already on main; main subsequently reworked that code in #635 (08032dc, d14a4c8) into a generic single_h->invoke_dynamic flag driven by ml_single_preset.invoke_dynamic / ml_single_open_with_option ("invoke_dynamic", "true"). So the flexible-handling part is fully superseded, and the enum value collides with ML_NNFW_TYPE_LLAMACPP = 21 (Since 10.0) added in 28b5dfb — which is why GitHub reports CONFLICTING (2 files, 3 hunks). The nnstreamer-side executorch-llama subplugin does exist (ext/nnstreamer/tensor_filter/tensor_filter_executorch_llama.cc, still present), but executorch_support is 0 in every Tizen profile of nnstreamer.spec. What remains genuinely new is ~4 lines: the enum entry (renumbered), the ml_nnfw_subplugin_name[] entry, and the _ml_validate_model_file case; the test should be rewritten against ml_single_open_with_option + invoke_dynamic.

[High] Enum value collision / ABI — c/include/ml-api-common.h:78 (PR): ML_NNFW_TYPE_EXECUTORCH_LLAMA = 21. main already has LLAMACPP = 21 and TIZEN_HAL = 22 (Since 10.0). Merging as-is would silently alias two frameworks. Fix: rebase, assign = 23 after TIZEN_HAL, add (Since 11.0) (or the next release) to the Doxygen, and file the Tizen ACR as in 28b5dfb.
[High] Superseded implementation / merge conflict — c/src/ml-api-inference-single.c:141, 783-786, 856, 956, 1095-1103, 1340-1343 (PR): the whole is_flexible machinery (struct field, ml_single_set_info_in_handle override, _ml_single_invoke_validate_data skip, g_object_set (filter_obj, "invoke-dynamic", TRUE)) duplicates what #607 merged and #635 refactored into single_h->invoke_dynamic (main lines 144, 749-757, 1106, 1181, 1455). Conflicting hunks: the enum tail, the subplugin-name table, and the validate-data continue block. Fix: drop commit 1 and the single.c portion of commit 3; keep only the name-table entry and the _ml_validate_model_file case.
[Medium] Design mismatch with main's opt-in flexible mode — :1095-1103 forces dynamic invoke by nnfw type. main deliberately keeps invoke_dynamic an explicit preset/option (nnstreamer-tizen-internal.h:51), and LLAMACPP does not auto-enable it. If executorch-llama must always be dynamic, that should be discussed with the #635 authors and applied symmetrically, not hard-coded for one type. Also the PR unconditionally sets gst_info.num_tensors = 1U, whereas main only does so when the filter reports 0.
[Medium] Test uses the wrong API and has UB paths — tests/capi/unittest_capi_inference_single.cc:3186-3225 (PR): uses ml_single_open (cannot request dynamic mode on main); out_data and result are uninitialized yet used in ml_tensors_data_destroy / strncmp / g_info when earlier EXPECT_EQs fail; g_info ("%s", result) assumes NUL-termination; strncmp (result, prompt, ...) assumes the LLM echoes the prompt. Typo tokienizer. The DISABLED_ prefix and @brief are fine. Fix: rewrite following the llama.cpp pattern in unittest_capi_service_extension.cc:407-532 (#if defined(ENABLE_...), skip-if-model-missing macro, invoke_dynamic option), initialize pointers to NULL, guard cleanup.
[Low] Doxygen / ACR completeness — ml-api-common.h:78: /**< ExecuTorch Llama runner */ lacks the (Since X.0) marker every other entry carries; commit message and PR body do not mention the ACR.
[Low] Tizen packaging reality — nnstreamer's spec sets %define executorch_support 0 in all profiles, so the subplugin is never shipped on Tizen today. Adding a Tizen public enum for a subplugin that cannot be loaded there is premature; either gate it like the Android-only SNAP or land it together with enabling executorch in the Tizen spec.
[Low] Process — 4 commits with one already merged separately; marked [After #607] but never rebased after #607 merged; no CI results on the branch. Last maintainer comment (2025-03-18) asked to hold until the Tizen GBM release.

No back-door or suspicious behavior found.

anyj0527 requested review from again4you, gichan-jang, jaeyun-jung, jijoongmoon, myungjoo and songgot as code owners October 29, 2024 08:12

anyj0527 force-pushed the single-llama branch 2 times, most recently from 83c9879 to ed9a143 Compare October 29, 2024 08:24

taos-ci approved these changes Oct 29, 2024

View reviewed changes

songgot reviewed Oct 30, 2024

View reviewed changes

songgot approved these changes Oct 30, 2024

View reviewed changes

niley7464 reviewed Oct 30, 2024

View reviewed changes

niley7464 approved these changes Oct 30, 2024

View reviewed changes

myungjoo reviewed Oct 30, 2024

View reviewed changes

jaeyun-jung reviewed Oct 30, 2024

View reviewed changes

Comment thread c/include/ml-api-common.h

jaeyun-jung added the ACR required label Nov 11, 2024

jaeyun-jung added the DO NOT MERGE label Nov 20, 2024

anyj0527 added 2 commits February 5, 2025 17:18

[api] Let single handle flexible filter

1674c69

- Let single API handle flexible filter. Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

[api] Add a new nnfw EXECUTORCH_LLAMA

44f4dcb

- Add a enum for new nnfw `EXECUTORCH_LLAMA` Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

anyj0527 force-pushed the single-llama branch from ed9a143 to 8f8b2e4 Compare February 5, 2025 08:22

anyj0527 changed the title ~~[api] Let single handle flexible filter~~ [After #607][api/single] Support executorch-llama Feb 5, 2025

anyj0527 added 2 commits February 5, 2025 17:27

[api] Let single handle executorch-llama as flexible filter

2622c67

- Let single API handle executorch-llama as flexible filter thus do invoke-dynamic. Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

[test] Add a disabled test for executorch-llama

cd1847b

- Add a simple test to show how to use executorch-llama with single api Signed-off-by: Yongjoo Ahn <yongjoo1.ahn@samsung.com>

anyj0527 force-pushed the single-llama branch from 8f8b2e4 to cd1847b Compare February 5, 2025 08:29

anyj0527 marked this pull request as draft March 18, 2025 07:23

myungjoo-bot suggested changes Sep 7, 2026

View reviewed changes

Navigation Menu

Uh oh!

[After #607][api/single] Support executorch-llama - #574

[After #607][api/single] Support executorch-llama#574
anyj0527 wants to merge 4 commits into
nnstreamer:mainfrom
anyj0527:single-llama

anyj0527 commented Oct 29, 2024

Uh oh!

taos-ci commented Oct 29, 2024

Uh oh!

taos-ci left a comment

Uh oh!

songgot Oct 30, 2024

Uh oh!

songgot Oct 30, 2024

Uh oh!

songgot left a comment

Uh oh!

niley7464 Oct 30, 2024

Uh oh!

niley7464 left a comment

Uh oh!

myungjoo Oct 30, 2024

Uh oh!

Uh oh!

jaeyun-jung commented Nov 20, 2024

Uh oh!

myungjoo-bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

anyj0527 commented Oct 29, 2024

Uh oh!

taos-ci commented Oct 29, 2024

Uh oh!

taos-ci left a comment

Choose a reason for hiding this comment

Uh oh!

songgot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

songgot Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

songgot left a comment

Choose a reason for hiding this comment

Uh oh!

niley7464 Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

niley7464 left a comment

Choose a reason for hiding this comment

Uh oh!

myungjoo Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaeyun-jung commented Nov 20, 2024

Uh oh!

myungjoo-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants