Skip to content

[single] Refactor setting tensors info in ml_single_open_custom - #661

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

[single] Refactor setting tensors info in ml_single_open_custom#661
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:refactor_single_open_custom

Conversation

@songgot

@songgot songgot commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

Improve ml_single_open_custom's framework-specific in/out tensors info settings handling to a table-based approach.
Improve code readability.

@songgot
songgot force-pushed the refactor_single_open_custom branch 2 times, most recently from c3d4dcb to 2d6a094 Compare November 18, 2025 00:41
@songgot songgot changed the title [single] Refactor: setting tensors info in ml_single_open_custom [single] Refactor setting tensors info in ml_single_open_custom Nov 18, 2025
Improve ml_single_open_custom's framework-specific in/out tensors info
settings handling to a table-based approach.

Improve code readability.

Signed-off-by: hyunil park <hyunil46.park@samsung.com>

@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 replaces the if (nnfw == TF || SNAP || PYTORCH || TRIX || NCNN) ... else if (nnfw == ARMNN) block in ml_single_open_custom with a static const table (requires_in_info / requires_out_info) plus a lookup loop. I compared every path against upstream/main (whose copy of this block is identical to the PR base; git merge-tree is clean): required-group frameworks still fail with ML_ERROR_INVALID_PARAMETER before any property is set when either info is missing, in/out are still set in the same order via the same helper, ARMNN still treats both as optional, and the resolved local nnfw is used as before. Behavior is equivalent; only log text changes. Non-blocking comments below.

  1. [Low] gst-indent violationsc/src/ml-api-inference-single.c:1154, 1157-1158, 1164, 1166-1168, 1173: over-long status = ml_single_set_inout_tensors_info (...) lines (original wrapped after status =), continuation indented 14 instead of 12 spaces, _ml_error_report_continue misaligned at 1166, _ml_logi("...") missing the GNU space before ( and >100 columns, and two consecutive blank lines at 1043-1044. Note indent.sh in CI only emits warnings, so this must be fixed manually with the repo's indent flags.
  2. [Low] New _ml_logi fires on the most common path:1172-1174: every framework not in the table (tflite, nnfw, snpe, onnx, tvm, tensorrt, qnn, llama.cpp, custom, ...) now logs "not found in tensors info table, using default behavior" on every open, which reads as if something is missing. Drop the else branch (matches the original) or downgrade to _ml_logd with wording like "no tensors-info requirement for '%s'".
  3. [Low] ARMNN diagnostics lost specificity:1156-1158, 1166-1168: the old ARMNN branch mentioned that infos are optional and included fw_name; both branches now share a generic message. Return codes are unchanged. Suggest including fw_name in the shared message.
  4. [Low] Naming / Doxygen:1018-1042: typedef nnfw_tensors_info lacks the repo's _s suffix, and the struct doc uses @param for members (Doxygen will warn); document members inline with /**< ... */.
  5. [Low] Readability gain is small — the table encodes only two cases (both required / both optional) with no asymmetric entries, yet adds a struct, 6 rows, three locals, a loop, and a found_in_table flag (+21 lines). A small helper returning an enum would achieve the same with less machinery; fine if asymmetric requirements are anticipated.
  6. [Low] Overlap with #651 — your open #651 extracts this exact region into helpers; whichever merges second will conflict. Consider folding this change into #651 or rebasing one on the other explicitly.
  7. [Info] Test coverage gap (pre-existing) — no unit test reaches the "required info missing -> ML_ERROR_INVALID_PARAMETER" path with a valid model, so a slip in this block would pass CI. Optional follow-up: a test opening a valid model with a required-info nnfw and NULL infos, skipped when the framework is unavailable.

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.

2 participants