search

Samsung / ONE

On-device Neural Engine
Other
439 stars 157 forks source link

Introduce sub command in `one-build` tool #7259

Open jyoungyun opened 3 years ago

jyoungyun commented 3 years ago

What

I suggest to introduce sub command in one-build tool.

Why

The current one-build command line tool is a kind of driver, so this invokes each tool based on configuration file.

The current way:

$ one-build --config <config file>
$ cat ./<config file>
[one-build]
one-import-tf=False
one-import-tflite=False
one-import-bcq=False
one-import-onnx=True
one-optimize=False
one-quantize=False
one-pack=False
one-codegen=False

[one-import-onnx]
input_path=./mbv2_05_relu_sigmoid.onnx
output_path=./mbv2_05_relu_sigmoid.circle

In the current way, user always has to put the configuration file no matter which tool is used. If sub command and command line options are introduced, the usability will be better. It still supports the existing configuration file usage.

seanshpark commented 3 years ago

sub command

Please describe more about sub command. I cannot get what you want to do.

seanshpark commented 3 years ago

If sub command and command line options are introduced, the usability will be better

How is it better? What is better? I can't catch what is before and after...

lemmaa commented 3 years ago

Considering the initial design of ONE-CMDS, I understand this proposal as following two cases.

The first of them is basically to change the configuration of the install package. Currently, install package links only one-build to /usr/bin and exposes it to users. However, by changing this, individual commands such as one-import , one-optimize , and one-quantize are additionally linked to /usr/bin so that they can be used individually.

Another aspect, perhaps, is to make the execution of one-build, one-import, one-optimize, one-quantize etc available in one <subcommand> format, like one build, one import, one optimize, one quantize... Perhaps a way to add a new driver called one that calls another module can be considered.

jyoungyun commented 3 years ago

Please describe more about sub command. I cannot get what you want to do.

one-build {import-tf | import-onnx | quantize | optimize | ...} [Options]

The sub command means the second parameter like {import-tf | import-onnx | quantize | optimize | ...}.

How is it better? What is better? I can't catch what is before and after...

User always has to put the configuration file no matter which tool is used. But if sub command and command line options are introduced, user can use one-build command without configuration file. User can call this tool with input parameter.

before:

# cat > a.cfg << EOF
> [one-build]
> one-import-tf=True
>
> [one-import-tf]
> input_path=/path/to/inception_v3.pb
> output_path=inception_v3.circle
> EOF
$ one-build -c a.cfg

after:

$ one-build import-tf -i /path/to/inception_v3.pb -o inception_v3.circle

However how to support this change is not determined yet. I will update comments again.

seanshpark commented 3 years ago

after:

$ one-build import-tf -i /path/to/inception_v3.pb -o inception_v3.circle

Users can

one-import-tf -i /path/to/inception_v3.pb -o inception_v3.circle ...

Question: what is the inconvenience for introducing this interface? Or is this "Nice to have" feature? Why I am asking: introducing new feature needs maintenance and we have resposibility to maintain that. Are YOU willing to?

lemmaa commented 3 years ago

https://github.com/Samsung/ONE/issues/7259#issuecomment-882970992

Please describe more about sub command. I cannot get what you want to do.

one-build {import-tf | import-onnx | quantize | optimize | ...} [Options]

I think it is better to treat one-build itself as one of the sub commands. It is a sub command that defines the workflow of other sub commands. In this case, the following format is suggested.

one build --> behaves as if we were calling `one-build`
one import --> behaves same as `one-import`
one optimize --> one-optimize 
...

Under this scenario, we need to introduce a new driver named one

seanshpark commented 3 years ago

Under this scenario, we need to introduce a new driver named one

Could you provide more about this? one of the main discussion point of this issue is to provide a way with options instead of .cfg file. So from your one introduction scheme, what comes after one optimize ? is it the options? or existing .cfg file or is it a new type of something? that is what is the full command that user can use?

lemmaa commented 3 years ago

https://github.com/Samsung/ONE/issues/7259#issuecomment-882980559

Question: what is the inconvenience for introducing this interface? Or is this "Nice to have" feature? Why I am asking: introducing new feature needs maintenance and we have resposibility to maintain that. Are YOU willing to?

It is probably expected to increase our controlability for interface management by limiting the command physically exposed to the user to one. At the same time, allowing users to use individual commands.

Maybe it's not that big of a difference. :)

If necessary, the old method can be deprecated once the new method is established. However, I think the existing method can still be usefully used to implement the command internally. Because it is so well implemented.

seanshpark commented 3 years ago

If necessary, the old method can be deprecated once the new method is established. However, I think the existing method can still be usefully used to implement the command internally. Because it is so well implemented.

There are two things, somewhat coflicts to me, discussed here.

@lemma, I don't care these are mixed in discussion but it's a little bit confusing on understanding your comment. on which point did you add it? is it to introduce sub command toone-buildorintroduce new command ` ?

jyoungyun commented 3 years ago

Question: what is the inconvenience for introducing this interface? Or is this "Nice to have" feature? Why I am asking: introducing new feature needs maintenance and we have resposibility to maintain that. Are YOU willing to?

As I know, if we introduce new feature, the old one will be deprecated for reducing the maintenance cost. And I will support to maintain new feature.

There are two things, somewhat coflicts, discussed here.

  • introduce sub command to one-build
  • introduce new command (name for that is posted as another issue)

Yes, this issue is mixed in two issue.

Initially, the purpose of this issue was to add sub-commands to one-build. But now it is no longer valid as we decided not to use one-build as the main driver shown to end users.

So the topic of this issue has been changed to adding a new interface.

This issue will be updated again after #7267 discussion is over.

And as I know, new feature will support both configuration file and sub-command options. @lemmaa Am I understanding correctly?

lemmaa commented 3 years ago

https://github.com/Samsung/ONE/issues/7259#issuecomment-882984687

Oh sorry, that was just an individual example line by line. It is not intended for sequential processing like a one-build.

So from your one introduction scheme, what comes after one optimize ? is it the options? or existing .cfg file or is it a new type of something? that is what is the full command that user can use?

I think that the options of individual commands are the same as before. We're just talking about changing the way individual commands are called.

Of course, more details such as whether to call one-import-tf as one import-tf or one import tf will need further discussion.

In the case of .cfg, I think that all commands can share it. In addition, each command can receive .cfg as command-line input, and at the same time, other options can also be received as command-line options, and override can be apply according to the rule. Of course, it is not expected that all of them will be implemented at once. I have no complaints about maintaining the current behavior of *cfg and options of one-cmds. :)

seanshpark commented 3 years ago

But now it is no longer valid as we decided not to use one-build as the main driver shown to end users.

I didn't notice this...

This issue will be updated again after #7267 discussion is over.

I think it would be better to reopen a new issue... as I wrote, two this are mixed up with a bit difference in interface that conflicts (to me...)

lemmaa commented 3 years ago

But now it is no longer valid as we decided not to use one-build as the main driver shown to end users.

I didn't notice this...

It seems that we are not communicating through a single channel at the moment. It's my mistake to proceed with the partial discussion on the internal messenger. ㅠㅠ

lemmaa commented 3 years ago

https://github.com/Samsung/ONE/issues/7259#issuecomment-882996142

And as I know, new feature will support both configuration file and sub-command options. @lemmaa Am I understanding correctly?

Yes, this is what I expected from the first design. :)