Skip to content

[API] Add ML LXM Service API(internal) for large model interactons - #646

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

[API] Add ML LXM Service API(internal) for large model interactons#646
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:dev_internal

Conversation

@songgot

@songgot songgot commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

This commit introduces the ML LXM Service API, a new C API designed to facilitate interactions with large-scale models such as Large Language Models (LLMs)

Comment thread c/include/ml-lxm-service-internal.h Outdated
Comment thread c/include/ml-lxm-service-internal.h Outdated
Comment thread c/include/ml-lxm-service-internal.h Outdated
Comment thread c/include/ml-lxm-service-internal.h Outdated
Comment thread c/src/ml-lxm-service.c Outdated
Comment thread c/src/ml-lxm-service.c Outdated
@songgot
songgot force-pushed the dev_internal branch 5 times, most recently from 0dd8e4b to 324bf4a Compare August 29, 2025 03:21
Comment thread c/include/ml-lxm-service-internal.h Outdated
@songgot
songgot force-pushed the dev_internal branch 3 times, most recently from 86a8dd5 to 9a94a33 Compare September 1, 2025 01:55
This commit introduces the ML LXM Service API, a new C API designed to
facilitate interactions with large-scale models such as Large Language
Models (LLMs)

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.

This API has a solid foundation and demonstrates clear design goals
and consistent implementation. It is particularly well-aligned with
its specific purpose of interacting with LLMs/LVMs.
With some small refactoring, it could become cleaner and more maintainable code.

* @return ML_ERROR_NONE on success.
* @note The callback parameter is mandatory and will be set during session creation.
*/
int ml_lxm_session_create (const char *config_path, const char *instructions, ml_service_event_cb callback, void *user_data, ml_lxm_session_h * session);

@hj210 hj210 Oct 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This function has a long parameter list. It would be better to reduce the number of parameters by using a structure type if possible.
For example,

typedef struct {
    const char *config_path;
    const char *instructions;
    ml_service_event_cb callback;
    void *user_data;
} ml_lxm_session_config_t;

int ml_lxm_session_create(const ml_lxm_session_config_t *config, 
                         ml_lxm_session_h *session) {
    if (!config || !session || !config->config_path || !config->callback)
        return ML_ERROR_INVALID_PARAMETER;
    
    return ml_lxm_session_create(config->config_path, config->instructions,
                                config->callback, config->user_data, session);
}

(This sample code is suggested by Cline)

@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 an internal "LXM" convenience layer over ml-service: a session (ml_service_new + ml_service_set_event_cb), a prompt (GString), and ml_lxm_session_respond which concatenates instructions + prompt into one tensor and calls ml_service_request. It merges cleanly against current main and CI is green. I read both new files against ml-api-service*.c / ml-api-common.c (destroying input_data right after ml_service_request is safe since the extension clones it), and checked packaging / Android build lists and the companion tests in #647.

  1. [Medium] NULL deref + stubc/src/ml-lxm-service.c:42-47: ml_lxm_check_availability writes *status without a NULL check (ml_lxm_check_availability (NULL) crashes) and always returns ML_LXM_AVAILABILITY_AVAILABLE, making the rest of the enum meaningless. Add the NULL check, and either implement a real check (e.g. check_feature_state (ML_FEATURE_SERVICE) -> SERVICE_DISABLED) or mark it @todo NYI in the header.
  2. [Medium] Unsynchronized free/read of instructionsml-lxm-service.c:209-222 (set_instructions: g_free + g_strdup) vs :250-254 (respond reads and appends). Calling set_instructions from another thread (e.g. from the event callback) while respond runs is a use-after-free. Add a GMutex to ml_lxm_session_internal (init in create, clear in destroy) and hold it in both places, as ml_service_s does.
  3. [Medium] No tests in this PR; #647's tests would not run in CI#647 gates the test binary on dependency('llama', required: false), which no CI workflow, spec, or debian rule provides, and the positive test skips without model files. Please land tests together with this code, and split them so NULL-parameter / missing-config / prompt create-append-destroy / set_instructions / respond negative cases build and run unconditionally under enable-ml-service, with only the token-generating path gated on llama / model availability.
  4. [Low] Header docsc/include/ml-lxm-service-internal.h:257 references ml_lxm_session_set_event_cb(), which does not exist (the callback is passed to ml_lxm_session_create); @example sample_lxm_service.c (line 12) names a file not in the repo; the "complete example" (~60-151) is C++ (<iostream>, static_cast) inside a C header — please make it plain C; enum values (~169-173) lack per-value /**< */ comments unlike every enum in ml-api-service.h.
  5. [Low] Prompt buffer has no terminatorml-lxm-service.c:268-270 sends full_input->len bytes. For flexible inputs the buffer is exactly that size; for static inputs a prompt that exactly fills the tensor has no NUL, and an oversized prompt fails with a bare ML_ERROR_INVALID_PARAMETER. Send len + 1 (or document the contract with the filter) and wrap the failure with _ml_error_report_return_continue.
  6. [Low] Style diverges from ml-api-service*.c — no check_feature_state (ML_FEATURE_SERVICE) at entry; bare return ML_ERROR_INVALID_PARAMETER; instead of _ml_error_report_return (...) with descriptive messages; g_malloc0 never returns NULL so the if (!s) / if (!p) branches (~76-79, ~140-141) are dead — use g_new0 (type, 1) as @jaeyun-jung suggested.
  7. [Low] ml_lxm_prompt_append_instruction is a byte-identical alias of append_text (:196-200), so "a" + instruction "b" yields "ab" with no separator or role marker. Give it real semantics or drop it for now. Also config_path is stored (:22, :81) but never read, and if (full_input) at :283 is always true.
  8. [Low] Build/packagingjava/android/nnstreamer/src/main/jni/Android-nnstreamer.mk:77-81 is not updated (LXM silently absent on Android); the header is neither in the c/include/meson.build install list nor in %files -n capi-machine-learning-tizen-internal-devel in the spec. If "in-tree only for now" is intended, please say so in the PR description.
  9. [Low] ml-lxm-service-internal.h:157 #include <stdlib.h> is unused; commit title typo "interactons".

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.

4 participants