Check for AVX instruction set during install for Precise compatibility #2269

mathmauney · 2019-08-23T19:37:03Z

Description

(Description of what the PR does, such as fixes # {issue number})
Check for AVX instruction set during install and warns the user that the Precise Engine will not be supported.

How to test

(Description of how to validate or test this PR)
Run installer on a machine that does not support AVX

Contributor license agreement signed?

CLA [ ] (Whether you have signed a CLA - Contributor Licensing Agreement

pep8speaks · 2019-08-23T19:37:06Z

Hello @mathmauney! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-29 15:40:55 UTC

devs-mycroft · 2019-08-23T19:37:27Z

Hello, @mathmauney, thank you for helping with the Mycroft project! We welcome everyone
into the community and greatly appreciate your help as we work to build an AI
for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require
a Contributor Licensing Agreement (CLA) before accepting any code
contribution. This agreement makes it crystal clear that along with your
code you are offering a license to use it within the confines of this project.
You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank
you!

mathmauney · 2019-08-23T19:40:15Z

Currently waiting on my CLA form.

Sorry about the line too long error, I disabled those in my PEP

forslund · 2019-08-24T06:33:48Z

dev_setup.sh

@@ -137,6 +137,22 @@ 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; then


Does this condition apply to the ARM platform as well? Since we have a precise build for that platform it might need to be excluded.

Didn't know about the ARM exception. Looks like that can be easily checked with $(uname -m) == 'arm'* so I've added that to the logic

Not easy to know :)

That looks like a good solution, great work with that.

Hey, we had some chat about this on the Forums. According to @domcross the ARM equivalent is called SIMD (Single Instruction, Multiple Data) and is available since ARMv7 (which includes the Cortex A53 used by the RaspberryPi and the Cortex A7 from the OrangePi).
Is it worth checking for SIMD on ARM?

The arm equivalents seem to have a bunch of different names. on the Pi it's neon and on the H64 it's called asimd so it might not be trivial to do a correct general check.

Ah, good to know, ignore me then :)
I think there's much less presumption that it will work on random ARM hardware anyway.

forslund · 2019-08-24T14:58:12Z

mycroft/client/speech/hotword_factory.py

@@ -326,6 +332,10 @@ def initialize():
                    hotword, module
                ))
                instance = None
+            except PreciseUnavailable:
+                LOG.warning('Settings prevent Precise Engine use,
+                            falling back to default.')


Hmm this multiline string doesn't seem to be valid. I suggest using

LOG.warning('Settings prevent Precise Engine use, ' 'falling back to default.')

Huh, thought that was what I changed it to already. Should be fixed now.

Fix long line string

mathmauney · 2019-08-24T21:35:21Z

CLA has been submitted

forslund · 2019-08-29T14:26:22Z

dev_setup.sh

+  Y)es, I want to use the PocketSphinx engine or my own.
+  N)o, stop the installation."
+      if get_YN ; then
+        sed -i "s/\"use_precise\": true/\"use_precise\": false/" config/


Is this sed line correct?
I get
sed: can't read config/: No such file or directory

Not sure which config you intend to edit here but you should probably make the adjustments in the ~/.mycroft/mycroft.conf config

Ah yeah that's where I had a test copy of a .conf.

It looks like there isn't any automatically generated one so we would have to deal with that.

Would the system level one be a better spot since you either will or will not have AVX for the entire system rather than at a user level?

Indeed might be better

Ok that should be done, it worked when I tested on a new system if I forced it to take that path. I don't have any machines that aren't ARM or AVX-compatible to really test on though.

dev_setup.sh

krisgesling

Hey there, sorry this dropped off my radar for a bit. It's a great addition though.

What do you think about the following?

krisgesling · 2020-11-20T06:56:25Z

dev_setup.sh

+          if [[ ! -e /etc/mycroft/ ]]; then
+            $SUDO mkdir /etc/mycroft
+          fi
+          $SUDO cp $TOP/mycroft/configuration/mycroft.conf /etc/mycroft/


Do we need to copy the default config across?
The default config will be used in its current location, and a config at /etc/mycroft/mycroft.conf would override it, (details here in our docs).

So wondering if we:

create the directory

create an empty json file if it doesn't exist

use jq to add/update the specific value

jq -c '. + { "use_precise": true }' mycroft.conf > tmp.$$.conf && mv tmp.$$.conf mycroft.conf

krisgesling · 2020-11-20T06:57:51Z

dev_setup.sh

+          if [[ ! -e /etc/mycroft/ ]]; then
+            $SUDO mkdir /etc/mycroft
+          fi


OPTIONAL
For creating the directory, personally I use mkdir -p instead of using a conditional. So these 3 lines could be simplified to:

$SUDO mkdir -p /etc/mycroft

krisgesling · 2021-05-28T12:21:20Z

Closing in favor of #2910

mathmauney added 2 commits August 23, 2019 15:30

Spellcheck

14c5201

Issue-2232 Add warning when AVX is not detected and fallback.

8458cde

devs-mycroft added the CLA: Needed Need signed CLA from https://mycroft.ai/cla label Aug 23, 2019

Fix line too long

2d323b6

forslund reviewed Aug 24, 2019

View reviewed changes

Ignore missing AVX for ARM systems

6a89093

forslund reviewed Aug 24, 2019

View reviewed changes

Update hotword_factory.py

af0b0fe

Fix long line string

devs-mycroft added CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) and removed CLA: Needed Need signed CLA from https://mycroft.ai/cla labels Aug 26, 2019

krisgesling mentioned this pull request Aug 29, 2019

Feature request: Check for AVX instruction set during install #2232

Closed

forslund reviewed Aug 29, 2019

View reviewed changes

mathmauney added 2 commits August 29, 2019 11:28

Edit system .conf and make if needed

5a5d7a6

Sed needs sudo

5de7d93

forslund reviewed Aug 29, 2019

View reviewed changes

dev_setup.sh Outdated Show resolved Hide resolved

krisgesling mentioned this pull request Jul 29, 2020

Handle devices without AVX instruction set MycroftAI/snapcraft-mycroft-core#21

Open

krisgesling added Status: To be reviewed Concept accepted and PR has sufficient information for full review Type: Bug - complex labels Sep 24, 2020

krisgesling reviewed Nov 20, 2020

View reviewed changes

krisgesling added Status: Change requested and removed Status: To be reviewed Concept accepted and PR has sufficient information for full review labels Nov 23, 2020

krisgesling changed the title ~~Feature/issue 2232~~ Check for AVX instruction set during install for Precise compatibility Apr 30, 2021

krisgesling mentioned this pull request May 28, 2021

Check for the AVX instruction set during install #2910

Merged

krisgesling closed this May 28, 2021

Check for AVX instruction set during install for Precise compatibility #2269

Check for AVX instruction set during install for Precise compatibility #2269

Uh oh!

Conversation

mathmauney commented Aug 23, 2019

Description

How to test

Contributor license agreement signed?

Uh oh!

pep8speaks commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-08-29 15:40:55 UTC

Uh oh!

devs-mycroft commented Aug 23, 2019

Uh oh!

mathmauney commented Aug 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathmauney commented Aug 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krisgesling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krisgesling commented May 28, 2021

Uh oh!

Uh oh!

pep8speaks commented Aug 23, 2019 •

edited

Loading