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.

Sign up for GitHub

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

Jump to bottom

Introduce prescription for deepsparse inference engine from Neural Magic #18515

Merged
merged 1 commit into from Oct 8, 2021
Merged

Introduce prescription for deepsparse inference engine from Neural Magic #18515

merged 1 commit into from Oct 8, 2021

Conversation

pacospace
Copy link
Contributor

@pacospace pacospace commented Oct 7, 2021 •
edited

Signed-off-by: Francesco Murdaca fmurdaca@redhat.com

What type of PR is this?

/kind feature

Related issues or additional information of the supplied change

Related-To: neuralmagic/deepsparse#186

Description

Deepsparse engine to this version 0.7.0 only supports avx2 and avx512 (and avx512 with optional vnni instructions): https://github.com/neuralmagic/deepsparse#hardware-support

Thoth resolution engine will fail if a user is asking for a recommendation for a different CPU family/model or the build to deploy a model is happening in a cluster without correct cpu family/model.

cc @bnellnm

@sesheta sesheta added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 7, 2021
@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 7, 2021
@pacospace pacospace requested a review from fridex October 7, 2021 09:41
@pacospace pacospace force-pushed the add-deepsparse-prescription branch 2 times, most recently from a4122b9 to 5d9b392 Compare October 7, 2021 09:43
@pacospace pacospace changed the title Introduce prescription for deepsparse inference engine from Neural Magic WIP: Introduce prescription for deepsparse inference engine from Neural Magic Oct 7, 2021
@sesheta sesheta added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2021
@pacospace pacospace force-pushed the add-deepsparse-prescription branch 3 times, most recently from ecdbd3d to 8199b6c Compare October 7, 2021 09:55
@pacospace pacospace changed the title WIP: Introduce prescription for deepsparse inference engine from Neural Magic Introduce prescription for deepsparse inference engine from Neural Magic Oct 7, 2021
@sesheta sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2021
fridex
fridex reviewed Oct 7, 2021
View reviewed changes
Copy link
Contributor

@fridex fridex left a comment

Choose a reason for hiding this comment

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

I'm unsure about the approach taken here. If the processor will be AVX512 enabled but not AVX2 enabled, the DeepSparseAVX2Sieve will sieve all deepsparse<=0.7.0 and vice versa. It might be a good idea to merge these prescriptions for the desired functionality.

prescriptions/de_/deepsparse/deepsparse_avx2.yaml Outdated Show resolved Hide resolved
prescriptions/de_/deepsparse/deepsparse_avx2.yaml Outdated Show resolved Hide resolved
prescriptions/de_/deepsparse/deepsparse_avx2.yaml Outdated Show resolved Hide resolved
@pacospace
Copy link
Contributor Author

I'm unsure about the approach taken here. If the processor will be AVX512 enabled but not AVX2 enabled, the DeepSparseAVX2Sieve will sieve all deepsparse<=0.7.0 and vice versa. It might be a good idea to merge these prescriptions for the desired functionality.

Perfectly make sense, I will merge them!

fridex
fridex approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@fridex fridex 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 👍🏻

@sesheta
Copy link
Member

sesheta commented Oct 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fridex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2021
@pacospace
Copy link
Contributor Author

/hold

@sesheta sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021
@pacospace
Copy link
Contributor Author

/hold

I need to add AMD family and models https://en.wikichip.org/wiki/amd/cpuid, Zen 2 and Zen 3

@pacospace pacospace mentioned this pull request Oct 7, 2021
Closed
7 tasks
@pacospace pacospace force-pushed the add-deepsparse-prescription branch 2 times, most recently from 4d6c4f0 to 0021dbd Compare October 7, 2021 15:34
@sesheta sesheta added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 7, 2021
@pacospace pacospace requested a review from fridex October 7, 2021 15:35
Introduce prescription for deepsparse inference engine from Neural Magic
d375a89 
Signed-off-by: Francesco Murdaca <fmurdaca@redhat.com>
@fridex fridex mentioned this pull request Oct 7, 2021
Closed
@pacospace
Copy link
Contributor Author

/unhold

@sesheta sesheta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2021
@sesheta sesheta merged commit 78e7c5e into thoth-station:master Oct 8, 2021
@fridex fridex mentioned this pull request Oct 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshad16 harshad16 Awaiting requested review from harshad16

@KPostOffice KPostOffice Awaiting requested review from KPostOffice

@fridex fridex Awaiting requested review from fridex

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pacospace @sesheta @fridex