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

Docker, requirements and amhf support #216

Merged
merged 23 commits into from
Mar 10, 2024

Conversation

GeoDerp
Copy link
Contributor

@GeoDerp GeoDerp commented Mar 2, 2024

No description provided.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

@davidusb-geek , will see how the test builds react to the change,

Copy link

codecov bot commented Mar 2, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 87.54%. Comparing base (468772d) to head (1a332c3).
Report is 27 commits behind head on master.

Files Patch % Lines
src/emhass/optimization.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   87.63%   87.54%   -0.10%     
==========================================
  Files           6        6              
  Lines        1707     1710       +3     
==========================================
+ Hits         1496     1497       +1     
- Misses        211      213       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

The current docker has been set up to use web_server requirements if standalone mode only. Is this right? Never actually thought about it before now.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

I'll think move the web_server requirements to be used on all modes makes more sense ? Or am I thinking in reverse and the web_server packages are only needed for the addon? @davidusb-geek

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

If it those three packages are required for all modes, could both requirments.txt and requirements_webserver be merged like on EMHASS-add-on?

@davidusb-geek
Copy link
Owner

If it those three packages are required for all modes, could both requirments.txt and requirements_webserver be merged like on EMHASS-add-on?

Yes probably the best solution.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

Sill testing. Just takes a while with pip.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

Unfortunately I don't believe I can finish testing this until tomorrow. Sorry about that.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

This is just a test. Will continue this further possibly tomorrow. (if build works, feel free to test and pull). Otherwise test when I'm next available.

Requirement change:
numpy<=1.26.0
to
numpy<=1.26.2

@davidusb-geek
Copy link
Owner

This is just a test. Will continue this further possibly tomorrow. (if build works, feel free to test and pull). Otherwise test when I'm next available.

Requirement change: numpy<=1.26.0 to numpy<=1.26.2

No don't worry no rush.
It seems that armv7 is failing again.
A problem with the SciPy version 1.11.3?

@davidusb-geek
Copy link
Owner

At some point in the past I was able to solve a lot of problems for arm arch by using this type of command to install packages:

RUN pip install --index-url=https://www.piwheels.org/simple --no-cache-dir -r requirements.txt

@davidusb-geek
Copy link
Owner

@GeoDerp could you help us understand where the persistent files are stored now. They are now inside the running docker for the add-on? Before they were saved in the share folder on HA

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 3, 2024

could you help us understand where the persistent files are stored now

The persistent data for a addon is in the /data folder. So DATA_PATH: '/data/' https://github.com/davidusb-geek/emhass-add-on/blob/07366be0a4044dff043587b441e3d798e77d245a/emhass/config.yml#L31C2-L31C22

And the options.json is also there automatically. Hopefully that helps.

The rest is being stored in /app/.

Do note that I believe /data is private to only the addon. The benefit is that uninstalling / updating / reinstalling the addon should manage the files correctly (would need to check to confirm). The downside you may not be able to access these files remotely.

You may wish to set this line back to /share if this isn't your desired result.

@davidusb-geek
Copy link
Owner

davidusb-geek commented Mar 3, 2024

No this seems nice. At some point we were having issues with data persistance after reboot and add-on update. So we implemented this manual data saving on the share folder. But if your implementation works fine which seems to be the case for now is good for me. That time was just my lack of skills on docker that made me use the manual approach. But I like this implementation more. We can still access the files if needed by going inside the container.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 3, 2024

@davidusb-geek , this is still in testing. Just presenting progress. One of the issues with numpy & scipy is their requirements with Blas/OpenBLAS. For some reason (32bit) armhf doesn't have that package. Therefore we are building this manually for that architecture.

There is a option to disable the use of blas for both. However I'm unsure if EMHASS uses this package. So I'm assuming it does.

I have also bumped the versions these 2 packages for testing purposes. I assume this could be bumped down and still work however. Will need to test functionality either way (if the build works).

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 3, 2024

At some point in the past I was able to solve a lot of problems for arm arch by using this type of command to install packages:

RUN pip install --index-url=https://www.piwheels.org/simple --no-cache-dir -r requirements.txt

This may still be the way to go. When testing I found this method to be slow (if not failing to pull). So I removed it for testing,

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 3, 2024

There is also an argument for how many users are still using 32bit versions on their Raspberry pi. A lot of these errors could be simply fixed by removing armhf support. (which I believe is 32 bit floating point for Debian)

Its to my understanding this will stop support for pi1,2,3(32bit). So I understand If this architecture wants to be kept.

The argument will be against how well EMHASS will run on a pi zero, 1,2,3 , and is it worth keeping support for the time being. Happy to work around whatever.
(This pr tries to keep armhf)

@davidusb-geek
Copy link
Owner

There is also an argument for how many users are still using 32bit versions on their Raspberry pi. A lot of these errors could be simply fixed by removing armhf support. (which I believe is 32 bit floating point for Debian)

Its to my understanding this will stop support for pi1,2,3(32bit). So I understand If this architecture wants to be kept.

The argument will be against how well EMHASS will run on a pi zero, 1,2,3 , and is it worth keeping support for the time being. Happy to work around whatever. (This pr tries to keep armhf)

Yes I understand the argument. It there is no simple solution we will have no other choice than drop support for armhf.
But let's try to keep at least armv7 alive ;-)

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 4, 2024

@davidusb-geek , realized why I was so confused. the armhf image of home-assistant/armhf-base-debian was set to use packages from armel. I don't know why. but the latest commits added a line to check armhf and change package architecture to armhf.

Currently testing this and a version with --index-url=https://www.piwheels.org/simple to see if there is any difference.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 4, 2024

Last commit, found that setting --index-url= overrides pypy making the non arm architectures to struggle. Setting an if statement to only use https://www.piwheels.org/simple on armhf and armv7. Fixes that solution.

It looks like all architectures work now. both arm builds take only 10m now also. (I think that's thanks to the new numpy/scipy versions)

@davidusb-geek
Copy link
Owner

Great! Nice finding of that solution

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 4, 2024

Just doing final testing now and ill do the last commit. (ill do a pr for EMHASS-Add-on also)

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 5, 2024

fyi, still testing on: https://github.com/GeoDerp/emhass
I may want to try and use armhf-base-raspbian for the armhf build. This will however May require a more complex Docker or a dedicated Docker file for armhf. We will see.

Edit: added raspbian, seems to work well so far with native armhf support.

- added Raspbian
- Dev Container actions better match Docker's EMHASS install and run  
- added --no-deps  and --force-reinstall to emhass pip
@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 5, 2024

armhf seems to run without any faults. Will do a more thorough test tomorrow.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 6, 2024

Testing process:

  • (Green: tested and success )

  • (Red: Untested/ tested and failed )

  • armhf

+Standalone

inc. all lp_solver, with patch for pulp cbc

+Add-On
  • amd64
+Standalone
+Add-On
  • aarch64
+Standalone
+Add-On 

inc. weather_forecast_method and lp_solver (separately)
Note: sometimes optimizations work even with broken PV sensor passed
Note: there was a 2 times where var_load couldn't be received from HA.
I could not replicate either of these issues easily so i'm assuming its on my side. Probably be good to check.

  • arm/v7 (did a quicker test)
+Standalone

inc. weather_forecast_method

+Add-On

inc. lp_solver

Testing Info

  • Testing: Perfect Optimization, Advanced ML Tasks, Publish Optimization Results, ML forecast model fit, ML forecast model predict, ML forecast model tune. (Not MPC)
  • Parameters: default (ex. lp_solver=default/COIN_CMD)

I may not test arm/v7, we will see. When these are all ticked fill free to merge this PR.
Then after the EMHASS pip version bump we can see the build tests of EMHASS-Add-on#216 and Merge if successful.
This may take a few days.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 6, 2024

Ill see if I can test the other two tomorrow. @davidusb-geek if you have a good MPC example that could work for testing send it my way and ill use it to test MPC also.

Something like this?

curl -i -H 'Content-Type:application/json' -X POST -d '{"pv_power_forecast":[0, 70, 141.22, 246.18, 513.5, 753.27, 1049.89, 1797.93, 1697.3, 3078.93], "prediction_horizon":10, "soc_init":0.5,"soc_final":0.6}' http://localhost:5000/action/naive-mpc-optim

Let me know if there is anything else you will like me to test with above.

@davidusb-geek
Copy link
Owner

Ill see if I can test the other two tomorrow. @davidusb-geek if you have a good MPC example that could work for testing send it my way and ill use it to test MPC also.

Something like this?

curl -i -H 'Content-Type:application/json' -X POST -d '{"pv_power_forecast":[0, 70, 141.22, 246.18, 513.5, 753.27, 1049.89, 1797.93, 1697.3, 3078.93], "prediction_horizon":10, "soc_init":0.5,"soc_final":0.6}' http://192.168.3.159:5000/action/naive-mpc-optim

Let me know if there is anything else you will like me to test with above.

This example test is good enough for me for MPC.

@GeoDerp GeoDerp changed the title Docker, added requirements_webserver to container, removed requirements_addon Docker, requirements and amhf support Mar 7, 2024
@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 7, 2024

Was unable to work on this today. Will try and get it done for the time you usually merge on the weekend.

@davidusb-geek
Copy link
Owner

Was unable to work on this today. Will try and get it done for the time you usually merge on the weekend.

Yes exactly, I will be able to take a more in depth look at all this this weekend. Thanks for these efforts!

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 7, 2024

All goes well the test will be done tomorrow. Otherwise the day after (Aus time). 👍

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 8, 2024

@davidusb-geek , im probably pretty happy with these changes. Ill see If I can test the other two by the time you test your self.

@davidusb-geek
Copy link
Owner

@davidusb-geek , im probably pretty happy with these changes. Ill see If I can test the other two by the time you test your self.

So the color-code table is updated?

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 8, 2024

im looking into virtualization of aarch64 and arm/v7 now.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 9, 2024

@davidusb-geek , feel free to start testing this.

Copy link
Owner

@davidusb-geek davidusb-geek left a comment

Choose a reason for hiding this comment

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

There are some typos on develop.md but I will fix them on my side.

@davidusb-geek davidusb-geek merged commit 46d6849 into davidusb-geek:master Mar 10, 2024
15 of 17 checks passed
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.

2 participants