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

Remove check as nncf now a required dependency #664

Closed
wants to merge 3 commits into from
Closed

Conversation

echarlaix
Copy link
Collaborator

@echarlaix echarlaix commented Apr 16, 2024

nncf added in openvino extra in #586

cc @helena-intel @eaidova

@echarlaix echarlaix marked this pull request as ready for review April 16, 2024 13:36
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@helena-intel
Copy link
Collaborator

helena-intel commented Apr 16, 2024

NNCF is a standard dependency in the [openvino] extra now, but from my perspective that doesn't make it a "required dependency". You can still use optimum-intel without NNCF. It's a pretty common use-case for me to convert models offline and then in the inference environment just do pip install optimum-intel openvino.

[EDIT] I see that with optimum-intel from source NNCF is already required just for importing . That is unexpected to me. I mentioned in the linked PR too that there was still a method to install without NNCF.

@echarlaix
Copy link
Collaborator Author

NNCF is a standard dependency in the [openvino] extra now, but from my perspective that doesn't make it a "required dependency". You can still use optimum-intel without NNCF. It's a pretty common use-case for me to convert models offline and then in the inference environment just do pip install optimum-intel openvino.

[EDIT] I see that with optimum-intel from source NNCF is already required just for importing . That is unexpected to me. I mentioned in the linked PR too that there was still a method to install without NNCF.

Yes it's the case since #638, added a fix in #668 in case we don't want to merge this PR (and make nncf officially a required dependency)

@echarlaix echarlaix closed this Apr 17, 2024
@echarlaix echarlaix deleted the rm-nncf-check branch May 29, 2024 09:54
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.

3 participants