MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.49k stars 1.27k forks source link

jq called before dependencies installed #3011

Closed in03 closed 2 years ago

in03 commented 2 years ago

Hey guys, I found a bug when installing Mycroft on my Raspberry Pi 4 8GB. AVX check is new for 64bit, so I didn't run into it until this install. It seems jq is required to update a field in mycroft.conf but installed as a dependency later.

jq called here: https://github.com/MycroftAI/mycroft-core/blob/039c84ee818529a7a37c1b0f3f3cb27e00c54b78/dev_setup.sh#L194

dependencies installed later: https://github.com/MycroftAI/mycroft-core/blob/039c84ee818529a7a37c1b0f3f3cb27e00c54b78/dev_setup.sh#L468

jq command seems to be malformed too. After manually installing, getting this error: parse error: Invalid numeric literal at line 1, column 3

I'm happy to try a pull request, but not super familiar with jq.

forslund commented 2 years ago

Thanks for reporting, we'd be happy to receive a PR. Easiest maybe to set a flag at the current location and then after the dependencies has been set it can be checked and installed if needed.

In my tests the jq command listed here works, can you check the content of your /etc/mycroft/mycroft.conf?

I'm a bit surprised that you end up here. looks like it should not be for arm platforms.

What does uname -m report on your device?

in03 commented 2 years ago

Hi Forslund, thanks for the reply :smiley: Sure! I'll give that flag suggestion a shot.

As far as your questions: I'm running a 64-bit build of DietPi on a Raspberry Pi 4 8GB uname -m comes back with 'aarch64' as expected.

DietPi has an optimized install for Mycroft, so it handles the mycroft.conf at /etc/mycroft/mycroft.conf a little differently. I think mostly to bypass dependency on pulseaudio and make moving user-data easier. Other than that, I haven't added anything custom to my configuration, so it's all stock-standard. But you're right, I tried the install again from my x86 laptop and forced the script to run that jq line and it executes fine. It might have been something DietPi specific or I broke it when I was looking, but will confirm.

The if/else statement for that AVX check is: if ! grep -q avx / proc/cpuinfo && ! [[ $(uname -m) == 'arm'* || $(uname -m) == 'aarch64' ]]; then echo "The Precise Wake Word Engine requires the AVX instruction set which is not supported on your CPU. Do you want to fall back to the PocketSphinx engine? Advanced users can build the Precise engine with an older version of TensorFlow (v1.13) if desired and change use_Precise to true in mycroft.conf"

I dug through AVX related commits and if I've got this right, the overall point of that check is to ensure Tensorflow is buildable? Since that's what Precise depends on. If there's no AVX on Intel/AMD it'll fail, ARM 32-bit will build (SIMD guaranteed), and aarch64 will fail because it has no official python Tensorflow build? Would it be worth having a slightly different prompt for aarch64 users that doesn't mention AVX since that's confusing to users?

I also noticed that a pull request for a bug mentioned here was in talks but I don't think it ever happened. The jq line should be setting use_precise = false to fallback to PocketSphinx, but it's still set to true.

The comments on that commit mentioned mycroft.conf precedence issues, which confused me a bit. What precedence do each of the mycroft.conf files take? Am I right in understanding that the system level conf should override the default level conf? If so, changing true to false should be all we need.

I'll post back with a pull-request that I'm happy to change upon suggestion :smile: :+1:

forslund commented 2 years ago

Sorry, apparently I was looking at an old branch. Doesn't the current if statement allow for aarch64 as well? Which should be correct, there is an aarch64 build of precise (with custom tensorflow build) (https://github.com/MycroftAI/precise-data/tree/dist/aarch64) which should work. (tested on ubuntu for the pi)

Your understanding is correct, the system level config /etc/mycroft/mycroft.conf will override the one included in this repo.

Yeah I was also wondering about that, it looks like it should be false.

in03 commented 2 years ago

Oh good! My mistake then. I was reading that 'if' statement wrong... But I tested and you're right. Since the 'if' statement doesn't prompt for aarch64, I must have absent-mindedly selected 'yes' later in the install to switch to the master branch and then lost the new fix on the dev branch for aarch64. Then when I cancelled the install to check something before running it again, I'd unknowingly changed branches and triggered the AVX prompt.

I tested my changes though and looks good, so submitted a pull request :+1: I'm new to this, so hopefully I followed the contributing guide right :sweat_smile: Thanks for the help @forslund!

forslund commented 2 years ago

Resolved by #3012, closing.

Thanks!