Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[onert] Let's remove default value from onert_train/args #12520

Closed
zetwhite opened this issue Jan 24, 2024 · 7 comments · Fixed by #12528, #12529, #12545 or #12556
Closed

[onert] Let's remove default value from onert_train/args #12520

zetwhite opened this issue Jan 24, 2024 · 7 comments · Fixed by #12528, #12529, #12545 or #12556

Comments

@zetwhite
Copy link
Contributor

zetwhite commented Jan 24, 2024

Background

Motivation1

For now, onert_train can set training info in 2 ways.

  • using model file (by circle file metadata)
  • using onert_train command line input

Since command line input always has a default value, onert_train always choose to use the cmd line info instead of choosing the model file's.

So, Let's

  • remove the default value(info)
  • make it choose to use the model file training info, if training info is not given from cmd line.

Motivation2

While working on motivation1, I found out that

  • onert_train/args just save an integer value
  • onert_train main does parsing - from an integer value to the corresponding enum NNFW_TRAIN_*.

Since onert_train/args holds an integer value, It is hard to remove the default value. More exactly, it is hard to represent that 'nothing is given'.

Additionally parsing (integer to NNfW_TRAIN enum) is not a kind of training logic, IMO it is better to remove it from the main.


Suggestion

In such background, I suggest this direction: #12445

AS-IS

  • onert_train/args: hold an integer value
  • onert_train/onert_train: parse integer value to NNFW_TRAIN_* enum and always overwrite session's training info.

TO-BE

  • onert_train/args : directly hold an enum NNFW_TRAIN_* and to represent nothing is given use std::optional.
  • onert_train/args : check cmd line parameter is given or not, and then decide whether to overwrite the session's training info or not.

Side effects of my suggestion

To merge the draft, it expects some additional changes :

  • Since std::optional is introduced in C++17, C++17 needs to be used to compile onert_train.
  • Since Boost v1.58.0 used in android-build does not support C++17, boost version has to be upgraded.
@zetwhite
Copy link
Contributor Author

@Samsung/one_onert

Could you briefly check draft #12445 and leave any opinion about the change?
I'd like to ask if it is fine to use C++17 for onert_train and upgrade the boost version to the latest.

@jyoungyun
Copy link
Contributor

I'd like to ask if it is fine to use C++17 for onert_train and upgrade the boost version to the latest.

👍

@zetwhite
Copy link
Contributor Author

zetwhite commented Jan 25, 2024

I asked @Samsung/one_onert opinion through another channel.

There are some opinions like :

  • It is okay to use C++17 in onert/tools.
    • (note) Since runtime/onert, runtime/lib is related with tizen release, it can NOT use C++17. (Tizen doesn't support C++17 officially. )
  • boost version upgrading have to be careful.
    • Because ONE-frontend has been used boost(not now)
    • There might be other libraries that depends on boost version.

Conclusion :

#12525 (upgrading boost version) merged and it looks fine.

So, There is no big problem to proceeding the draft i suggested #12445.
I'll make PRs based on #12445 .

@zetwhite
Copy link
Contributor Author

zetwhite commented Jan 25, 2024

TODO

@YongseopKim
Copy link
Contributor

What happened this issue? 😆

@jyoungyun
Copy link
Contributor

jyoungyun commented Feb 2, 2024

@YongseopKim

What happened this issue? 😆

@zetwhite linked each PR to this issue using a comment like this in the process to resolve: https://github.com/Samsung/ONE/issues/12520. It linked each PR to this issue, and if the PR was successfully merged, it would close this issue because it was linked to the PR. That's the why this issue was closed repeatedly. 😄

@zetwhite
Copy link
Contributor Author

zetwhite commented Feb 7, 2024

Now, onert_train runs with below policy:

  • onert_train expects that circle+ file is given - model file with training information.
  • If a user wants to train by setting training information manually, the User can update training information by cmd line option.
  • If a user wants to train using circle file (not circle+), the User can train it by manually setting training information by using cmd line option.
Case Model file info is given cmd line info is given onert_train Selected
1 X X train info is undefined, throw error
2 O (circle+) X model file info selected
3 O (circle+) O cmd line info is selected
4 X (circle, not circle+) O cmd line info is selected

@zetwhite zetwhite closed this as completed Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment