Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Check for the AVX instruction set during install #2910

Merged
merged 8 commits into from
Jun 1, 2021
Merged

Conversation

krisgesling
Copy link
Contributor

Description

Add a check for the AVX instruction set on non-ARM processors during install. If this required instruction set is not found it will warn the user that the Precise Engine will not be supported.

Replaces #2269
Thanks to @mathmauney for the original work on this.

How to test

Run installer on a machine that does not support AVX
or change line 140 so it is searching for something that definitely doesn't exist in the cpuinfo eg change "avx" to "avxzzz"

Contributor license agreement signed?

Yes - both original author and myself.

@krisgesling krisgesling added Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. Status: To be reviewed Concept accepted and PR has sufficient information for full review labels May 28, 2021
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 28, 2021
@krisgesling
Copy link
Contributor Author

Rebased - please also be sure to squash commits if merging.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

Merging #2910 (ff2db27) into dev (d474e79) will increase coverage by 0.00%.
The diff coverage is 57.14%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #2910   +/-   ##
=======================================
  Coverage   52.60%   52.60%           
=======================================
  Files         123      123           
  Lines       11000    11007    +7     
=======================================
+ Hits         5786     5790    +4     
- Misses       5214     5217    +3     
Impacted Files Coverage Δ
mycroft/client/speech/hotword_factory.py 70.52% <57.14%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d474e79...ff2db27. Read the comment docs.

Copy link
Contributor

@ken-mycroft ken-mycroft left a comment

Choose a reason for hiding this comment

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

Just confused about the 'arm' conditional.

@@ -174,6 +174,30 @@ This script is designed to make working with Mycroft easy. During this
first run of dev_setup we will ask you a few questions to help setup
your environment.'
sleep 0.5
if ! grep -q avx /proc/cpuinfo && [[ ! $(uname -m) == 'arm'* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive me but I am not sure of the objective. I understand the check for avx via cpuinfo but must admit the arm check is confusing me. What is going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As eltocino replied below, however it is the inevitable question - so I've added a comment to explain for future devs.

The AVX instruction set is an x86 construct. ARM has a range of equivalents, but I'm not sure which are supported or not by TF. So this is intended to catch the bulk of the support issues we get which is from people trying to install Mycroft on older or low-end x86 hardware.

@el-tocino
Copy link
Contributor

@ken-mycroft AVX is an x86 construct, ARM does not have it.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

Copy link
Contributor

@ken-mycroft ken-mycroft left a comment

Choose a reason for hiding this comment

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

Looks good. Go ahead.

@krisgesling krisgesling merged commit 0e2a780 into dev Jun 1, 2021
@krisgesling krisgesling deleted the feature/avx-check branch June 1, 2021 21:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: To be reviewed Concept accepted and PR has sufficient information for full review Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants