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

Turn command import into optional #936

Merged
merged 9 commits into from
Apr 3, 2023
Merged

Conversation

JingyaHuang
Copy link
Contributor

What does this PR do?

as per title.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 30, 2023

The documentation is not available anymore as the PR was closed or merged.

@fxmarty
Copy link
Contributor

fxmarty commented Mar 31, 2023

I am personally not in favor of this - basically some commands will not show depending on whether you have installed a package or not? Ideally you would want to see all possible commands, with errors raised if you miss a package for one.

@JingyaHuang
Copy link
Contributor Author

@michaelbenayoun WDYT?

@fxmarty What about my initial suggestion on not setting any default subcommand, and add them by register? Like registering onnx, tflite and onnxruntime in a register.py and in optimum-neuron override it with a same register.py but only register neuron associated commands. In this way when using the cli of optimum message will be related to submodule in optimum main, and in neuron, it will only be neuron related requirements. (although they are all optimum-cli, IMO it's fine not poping out onnx info whenusing optimum-cli under neuron env)

@michaelbenayoun
Copy link
Member

I agree with @fxmarty, what about doing this instead: each BaseOptimumCliCommand can define a check_requirements method, that is being run at in the right time, to fail if the command is ran without the requirement, and nothing happens otherwise?

@JingyaHuang
Copy link
Contributor Author

Can I merge this one? @michaelbenayoun @fxmarty

Copy link
Member

@michaelbenayoun michaelbenayoun left a comment

Choose a reason for hiding this comment

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

LGTM (as long as the tests pass)

optimum/commands/export/tflite.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@JingyaHuang JingyaHuang merged commit 0bf2c05 into main Apr 3, 2023
@JingyaHuang JingyaHuang deleted the optional-command-import branch April 3, 2023 13:22
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