[onert/api] Introduce nnfw_compile and nnfw_set_compiled_model_path APIs #12583

jyoungyun · 2024-02-01T02:39:15Z

This commit introduces nnfw_compile and nnfw_set_compiled_model_path.
It contains a detailed description of these APIs in the header file.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun jy910.yun@samsung.com
Co-authored-by: Sanggyu Lee sg5.lee@samsung.com

Related issue : #12505
Draft PR: #12504

This commit introduces nnfw_compile and nnfw_set_compiled_model_path. It contains a detailed description of these APIs in the header file. ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <jy910.yun@samsung.com> Co-authored-by: Sanggyu Lee <sg5.lee@samsung.com>

glistening · 2024-02-01T03:24:08Z

@jyoungyun I am not sure nnfw_compile and nnfw_set_compiled_model_path are clear.

Internally when @chunseoklee / @hseok-oh and I wrote codegen (q-circle → binary for npu), it was the 1st and the only one which transform something to another. But as quantizer is introduced later, I would like to use compile as broader meaning.

That is, in my mind, compile is the term for the whole end-to-end path like on-deivce compiler.

f-circle --(quantize)--> q-circle --(codegen)--> binary
`--------------------compile------------------------'

Thus, I would like to use nnfw_codegen and nnfw_set_codegen_path at this moment.

I would like to hear other's opinion.

Also, after we used nnfw_compile, we developed on-device training, and it has nnfw_train prefix for all training-realted APIs. Will use introduce another prefix? Or go w/o prefix?
Will we introduce another prefix? Or name w/o prefix?

YongseopKim

LGTM but code about codegen can be revised soon(or in the future).

I discussed it with @jyoungyun and heard that almost code has been written before.
That's why LGTM with some comment.

YongseopKim · 2024-02-01T04:45:16Z

runtime/onert/api/nnfw/src/nnfw_api_internal.cc

+    }
+
+    assert(_codegen_manager != nullptr);
+    _codegen_manager->exportModelPath(std::string(path));


This is just setter for _export_model_path, right? This is really confusing... In the future, this should be fixed, I think.

@YongseopKim Thank you for your comment. :) Can I ask what you want to fix?

(Above all, I don't say that this should be done right now. This is one of points to revise in the future.)

I talked to the name exportModelPath().

_codegen_manager->exportModelPath(std::string(path));

export is a verb word so the method can be translated to 'the manager exports with the model path and returns a something to be export'.

That's why I'm saying to fix it. 😄

YongseopKim · 2024-02-01T04:45:58Z

runtime/onert/api/nnfw/src/nnfw_api_internal.cc

+    }
+
+    assert(_codegen_manager != nullptr);
+    auto export_model_path = _codegen_manager->exportModelPath();


This is just getter for _export_model_path.

This is not calculating(manipulating) path for model.

Let's revise this for sure in the future!

@YongseopKim

This is not calculating(manipulating) path for model.

I'm sorry but I couldn't catch your intention. Could you explain that in some detail?

YongseopKim · 2024-02-01T04:49:16Z

runtime/onert/api/nnfw/src/nnfw_api_internal.cc

+    auto export_model_path = _codegen_manager->exportModelPath();
+    // If the export_model_path is not set, it generates a compiled model path
+    // automatically.
+    if (export_model_path.empty())


These code(Set the proper model path with some vars) can be in CodegenManger.

@YongseopKim I agree to implement this function in CodegenManager. I will update it in another PR. :)

hseok-oh · 2024-02-01T06:38:21Z

Thus, I would like to use nnfw_codegen and nnfw_set_codegen_path at this moment.

I would like to hear other's opinion.

I have similar opinion with @glistening. Additionally, compile make confusion with runtime core's Compiler class.

glistening

LGTM. Let's revise later. The APIs are in nnfw_experimental.h. We may change it.

jyoungyun · 2024-02-01T07:33:12Z

@glistening @hseok-oh

Thus, I would like to use nnfw_codegen and nnfw_set_codegen_path at this moment.

I would like to hear other's opinion.

I have similar opinion with @glistening. Additionally, compile make confusion with runtime core's Compiler class.

I agreed with your opinion. I also thought this api name is not suitable for Codegen feature.
#12504 (comment)

If everyone agrees to modify this function to use codegen string rather than compile, I'd like to modify in this PR.
Do you agree to use codegen string instead of compile for API name?

hseok-oh · 2024-02-01T08:30:29Z

I agreed with your opinion. I also thought this api name is not suitable for Codegen feature. #12504 (comment)

If everyone agrees to modify this function to use codegen string rather than compile, I'd like to modify in this PR. Do you agree to use codegen string instead of compile for API name?

Agree

YongseopKim · 2024-02-01T08:38:18Z

Agree

glistening · 2024-02-01T08:43:15Z

Agree. codegen is better than compile. Though I am still not sure, but it is okay since it is under experimental.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <jy910.yun@samsung.com>

jyoungyun · 2024-02-01T09:11:44Z

If you want to update this PR's title, please let me know. Currently, I just added a new commit for this change.

YongseopKim

LGTM 👍

jyoungyun · 2024-02-02T04:37:23Z

@glistening PTAL

jyoungyun · 2024-02-05T03:10:19Z

@glistening PTAL, again.

glistening

LGTM

glistening · 2024-02-05T03:37:41Z

@jyoungyun Could you update (or May I edit) the commit title as the function names are changed? It may be better to update by myself, since it already has 2 approvals.

hseok-oh · 2024-02-05T05:01:17Z

I've updated title and commit.

This commit implements codegen function in onert_run. As discussed below, the `compile` string of functions and variables has been changed to `codegen`. Samsung#12583 (comment) ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <jy910.yun@samsung.com>

This commit implements codegen function in onert_run. As discussed below, the `compile` string of functions and variables has been changed to `codegen`. #12583 (comment) ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <jy910.yun@samsung.com>

jyoungyun requested a review from a team February 1, 2024 02:39

jyoungyun added the PR/ready for review It is ready to review. Please review it. label Feb 1, 2024

jyoungyun mentioned this pull request Feb 1, 2024

[onert] Introduce on-device codegen feature #12505

Closed

6 tasks

YongseopKim previously approved these changes Feb 1, 2024

View reviewed changes

glistening previously approved these changes Feb 1, 2024

View reviewed changes

jyoungyun added PR/NO MERGE Please don't merge. I'm still working on this :) and removed PR/ready for review It is ready to review. Please review it. labels Feb 1, 2024

Change function prefix to codegen

830fb35

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <jy910.yun@samsung.com>

jyoungyun dismissed stale reviews from glistening and YongseopKim via 830fb35 February 1, 2024 09:09

jyoungyun added PR/ready for review It is ready to review. Please review it. and removed PR/NO MERGE Please don't merge. I'm still working on this :) labels Feb 1, 2024

YongseopKim approved these changes Feb 1, 2024

View reviewed changes

hseok-oh approved these changes Feb 1, 2024

View reviewed changes

glistening approved these changes Feb 5, 2024

View reviewed changes

hseok-oh merged commit 356901d into Samsung:master Feb 5, 2024
9 checks passed

jyoungyun deleted the onert/introduce_nnfw_compile branch February 6, 2024 06:02

jyoungyun mentioned this pull request Feb 6, 2024

[tools/onert_run] Implement codegen function #12584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[onert/api] Introduce nnfw_compile and nnfw_set_compiled_model_path APIs #12583

[onert/api] Introduce nnfw_compile and nnfw_set_compiled_model_path APIs #12583

jyoungyun commented Feb 1, 2024

glistening commented Feb 1, 2024

YongseopKim left a comment

YongseopKim Feb 1, 2024

jyoungyun Feb 1, 2024

YongseopKim Feb 1, 2024

YongseopKim Feb 1, 2024

YongseopKim Feb 1, 2024

YongseopKim Feb 1, 2024

jyoungyun Feb 1, 2024

YongseopKim Feb 1, 2024

YongseopKim Feb 1, 2024

jyoungyun Feb 1, 2024

hseok-oh commented Feb 1, 2024 •

edited

Loading

glistening left a comment

jyoungyun commented Feb 1, 2024

hseok-oh commented Feb 1, 2024

YongseopKim commented Feb 1, 2024

glistening commented Feb 1, 2024

jyoungyun commented Feb 1, 2024 •

edited

Loading

YongseopKim left a comment

jyoungyun commented Feb 2, 2024

jyoungyun commented Feb 5, 2024

glistening left a comment

glistening commented Feb 5, 2024 •

edited

Loading

hseok-oh commented Feb 5, 2024

[onert/api] Introduce nnfw_compile and nnfw_set_compiled_model_path APIs #12583

[onert/api] Introduce nnfw_compile and nnfw_set_compiled_model_path APIs #12583

Conversation

jyoungyun commented Feb 1, 2024

glistening commented Feb 1, 2024

YongseopKim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hseok-oh commented Feb 1, 2024 • edited Loading

glistening left a comment

Choose a reason for hiding this comment

jyoungyun commented Feb 1, 2024

hseok-oh commented Feb 1, 2024

YongseopKim commented Feb 1, 2024

glistening commented Feb 1, 2024

jyoungyun commented Feb 1, 2024 • edited Loading

YongseopKim left a comment

Choose a reason for hiding this comment

jyoungyun commented Feb 2, 2024

jyoungyun commented Feb 5, 2024

glistening left a comment

Choose a reason for hiding this comment

glistening commented Feb 5, 2024 • edited Loading

hseok-oh commented Feb 5, 2024

hseok-oh commented Feb 1, 2024 •

edited

Loading

jyoungyun commented Feb 1, 2024 •

edited

Loading

glistening commented Feb 5, 2024 •

edited

Loading