[codex] WLED: use repo-aware release checks for upgrades - #172481
[codex] WLED: use repo-aware release checks for upgrades#172481LordMike wants to merge 5 commits into
Conversation
|
Hey there @frenck, @mik-laj, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Note - this is not entirely ready. The PR is still draft for that reason. Discussion will continue in frenck/python-wled#2078 until that has been merged. |
| return True | ||
|
|
||
|
|
||
| async def async_get_releases_coordinator( |
There was a problem hiding this comment.
This is a bit problematic because this coordinator is never stopped when a user stops using the fork.
I'm wondering if it would be possible to update the coordinator to check which repositories the user is using and download data for all the repositories the user is currently using.
What do you think about this?
There was a problem hiding this comment.
Another solution is to register the repository in the release coordinator in async_setup_entry and then unregister it in the async_unload_entry function.
hass.data[WLED_KEY].register_repo(entry_id, coordinator.data.info.repo)hass.data[WLED_KEY].unregister_repo(entry_id)There was a problem hiding this comment.
As I see it though, the release coordinator remains in memory, yes (and we could definitely prune that) - but we're not actively polling the repo there. So if you add a device A, with repo A - and remove device A. Then repo A remains, but is not polled. The DataUpdateCoordinator base operates with listeners (each update entity listens), and when the last listener disappears, which it hopefully does, DataUpdateCoordinator stops polling.
I'll see if we can't clean out the dict regardless.
There was a problem hiding this comment.
Are you on to making WLEDReleasesDataUpdateCoordinator able to manage multiple repositories in one instance?
There was a problem hiding this comment.
So I looked into blindly going for entry setup/teardown, but it would introduce extra bookkeeping. On entry_setup ( a single device), we create the coordinator if not existing. But on teardown, we'd have to start reviewing if the coordinator is still in use.
It seems unlikely to be a problem, leaving hte coordinator around after the device has disappeared. It only lasts till next restart of the HA instance. And it really is only 1 instance per unique repository - so in practice its like.. at most 2..
There was a problem hiding this comment.
Hmm, a drawback here is that:
- there is one release coordinator, and it makes N requests on updates - if any 1/N requests fail, the entire check fails
- if an update entity forces a recheck, it will recheck all repos
I'm partial to the one instance per repo instead, keep it lean.. but then try to solve for cleanup - either the temporary bit like disabling the update entity, or the more permanent bit that is removing a WLED device (entry) entirely. It could be solved by a set of entry_ids in each release coordinator, and when that set is empty, we remove it.
The release coordinator already tracks subscriptions, which each update entity uses. We could in our WLED release coordinator, expose the count of subscriptions, and if after removal this becomes 0, we clean up the instance entirely. Hopefully we can then tweak disabling of update entities to unsubscribe, which in turn will make stuff auto-cleanup.
There was a problem hiding this comment.
There is something on .. what if we have 2 wled devices (same repo), and we do a force update on all their update entities at the same time. We will then do two forceful data updates, which means two queries for releases, parsing of those, paginating those, and so on..
If you had 10 or 100 wled devices, we might hit GH rate limits, even.
... something like a time limit on rechecks? "minimum 60s since last check" -style.
There was a problem hiding this comment.
there is one release coordinator, and it makes N requests on updates - if any 1/N requests fail, the entire check fails
We can add better error handling similarly to what is done in the system monitor integration.
core/homeassistant/components/systemmonitor/coordinator.py
Lines 189 to 200 in 840243d
if an update entity forces a recheck, it will recheck all repos
I think this is a minor issue because at most the user will have more up-to-date data.
There is something on .. what if we have 2 wled devices (same repo), and we do a force update on all their update entities at the same time.
I think that the coordinator debouncer will work here, it will prevent the same data from being downloaded multiple times in a short period of time.
I'm partial to the one instance per repo instead, keep it lean.. but then try to solve for cleanup - either the temporary bit like disabling the update entity, or the more permanent bit that is removing a WLED device (entry) entirely. It could be solved by a set of entry_ids in each release coordinator, and when that set is empty, we remove it.
I'm not sure what that would look like, but we could try to implement it. I proposed my solution because it sounded relatively simple to me. It requires the least amount of code changes to get it working now. I will open a PR with my proposal so that we can see what it looks like in full and then we can compare.
There was a problem hiding this comment.
Ok, I'll move closer to a single instance of update coordinator, and then tracking repos in it.
Good catch. We need to override core/homeassistant/helpers/update_coordinator.py Lines 696 to 705 in e5fad17 It would be good to fix this in a separate PR. |
|
I also reconfigured my wled device, in the hopes it would trigger a recheck.. And then the config page gave me a string placeholder like "reconfigure_succeeded".. Looks like the config flows could also need a hand.. :P |
Things we could fix:
|
|
Good list - a few thoughts: update_entity: Already handled in #172517. Config flow texts: I haven't personally observed this issue. Could you double-check that all translation files are compiled/up-to-date on your end? Update interval: The 3h interval supports both stable and nightly releases in the same coordinator, so it isn't just probing for once-a-year stable events - it's 8 requests per day per repo. That said, I agree any interval above 1h is reasonable. With release_summary / release_notes: Good idea. I think we need a separate PR to python-wled that adds this. Stable vs. prerelease tracking: Good point. If a device is running a prerelease, we should probably keep offering prerelease updates rather than silently landing on stable. There's a related discussion in #160270 - worth aligning with whatever comes out of that. |
Quick note on this. HA data shows 34k installations of WLED. Lets assume close to 100% have 1+ devices, on currently one repository. Thats 34k req/3h, or ~10k/h. I've acquired stable release dates in this table:
That's potentially, on average (assuming stable users) 100 days of 10k req/h, before a "hit" is made. Beyond this, and the whole "lets hammer a free service like Github" thing, there's these points for slower updates, like checking once daily or once every few days:
|
10k/h requests is nothing for GitHub. Really. But we can bump it now to one day if we're adding support for multiple repos. It's best to do that in a separate PR so we can figure out what the best value is in separate thread. |
That PR was closed, so no further discussion is made. But if I read it right, the chosen option is essentially what is done today - the HA wled integration checks the current fw, and then picks the newest of the allowed set of (stable, unstable, nightly).. with the natural tendency to move towards stable. It is possible to make settings per subenty, so a quick cogwheel could be added to show this dropdown of release track to follow. Users wouldn't have to do anything, on device setup or first run of this new code, the current firmware version could be determined and then the track stored in the subentry setting. Then for all future runs, it just is what it should be. A user may then change it to f.ex. "stable", or "stable and beta and nightly", or ... |
|
I've changed the PR to revert to a single WLED Update Coordinator, which then holds the full set of release infos in a dict. Steps have been taken to ensure we track required repos properly, and that should github fail, we're not reverting back to no data at all. |
Breaking change
Proposed change
WLED upgrade checks now use repository-aware release lookup through
WLEDReleases, default towled/WLEDwhen the device does not report a repository, deduplicate release lookups per unique repo, and upgrade using the repo and release reported by the device API.This PR depends on frenck/python-wled#2078 and is opened to allow review from @mik-laj.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running:
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: