Add nws coordinator listeners #115836

kamiyo · 2024-04-19T05:47:49Z

Proposed change

This registers the coordinator updates for twice_daily and hourly forecasts in NWS, which should prevent stale data mentioned in the issues below.

This also updates the available check to account for these coordinators (using the exact same algorithm, which I'm going to take as correct for the time being).

Finally, added async_request_refresh() calls to these coordinators in the async_update() function.

I believe these changes should solve these issues (and maybe many other dupes): #115769, #111146, #115433

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes NWS Get_Forcasts returning old data #115769, NWS Weather integration not compatible with new get_forecasts service #111146, NWS twice daily "forecasts" are in the past #115433
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2024-04-19T05:47:55Z

Hey there @MatthewFlamm, mind taking a look at this pull request as it has been labeled with an integration (nws) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of nws can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign nws Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

MatthewFlamm · 2024-04-19T11:45:25Z

I have also been investigating this. I feel like the refresh machinery should be subscribed to inside the weather integration code, not here. We have an async_added_to_hass here, and if we remove it, I can get the code to work when testing live, but I'm struggling with the testing. I will put up a draft PR once I clean up my version so we can compare notes.

Edit: see #115857

MatthewFlamm

The tests need to be updated because the hourly forecast will now be updated the same number of times of the twice daily (at least in most tests).

MatthewFlamm · 2024-04-24T11:38:59Z

homeassistant/components/nws/weather.py

@@ -141,6 +141,8 @@ def __init__(
        latitude = entry_data[CONF_LATITUDE]
        longitude = entry_data[CONF_LONGITUDE]
        self.coordinator_forecast_legacy = nws_data.coordinator_forecast
+        self.coordinator_forecast_twice_daily = nws_data.coordinator_forecast


We don't need these, they are already available as self.forecast_coordinators["twice_daily"] and similar the hourly.

MatthewFlamm · 2024-04-24T11:39:31Z

homeassistant/components/nws/weather.py

@@ -160,6 +162,16 @@ async def async_added_to_hass(self) -> None:
                self._handle_legacy_forecast_coordinator_update
            )
        )
+        self.async_on_remove(
+            self.coordinator_forecast_twice_daily.async_add_listener(


Same comment as above, use the built in attribute for the forecast coordinators.

And several more places below.

MatthewFlamm · 2024-04-24T11:42:24Z

#115857 will take some time to mature, so this PR should move forward to make immediate fixes. Clean up of the code is happening inside the other PR.

kamiyo · 2024-04-24T15:37:59Z

@MatthewFlamm I'll work on the changes and test. In the meantime, should I remove the legacy forecast? Or still keep it in there for now?

MatthewFlamm · 2024-04-24T15:44:29Z

If you remove legacy_forecast the NWS tests will likely fail, which is what #115857 found.

kamiyo · 2024-04-26T03:04:16Z

@MatthewFlamm I'm having trouble understanding what exactly is failing on the unsuccessful test, if you get a chance to review it.

MatthewFlamm · 2024-04-26T11:41:56Z

I wonder if it is a bug or race condition in core. The failure clearly shows that weather.abc exists but then claims it doesn't in the error. If it is only failing for get_forecast, this is deprecated, so we could remove parametrization for that one in our integration?

Looking at this again, I think you could try removing legacy_forecast, since you have listeners for the other forecasts now.

kamiyo · 2024-04-27T06:23:52Z

I am not sure if this diagnosis is correct, but for the line 395 in the test, after the fast forward:

    # after additional 35 minutes data caching expires, data is no longer shown
    freezer.tick(timedelta(minutes=35))

Given the current available code, not only is data no longer shown, but the entity returns unavailable. And the service calling code does not allow you to call a service on entities that are not available, I think. Since I see this in the helpers/service.py:

        for entity in entity_candidates:
            if not entity.available:
                continue

So the NWS isn't being added to the list of entities to call the service on, and there are no other entities in the list, hence did not match any entities.

So what's the intended behavior when we call async_call on an unavailable entity? According to the test code, it should just return the snapshot, even if the data is stale. But I don't think that's good behavior. So maybe I should test with an assertRaises for the exception?

kamiyo · 2024-04-29T15:15:59Z

@MatthewFlamm in case you had an opinions about the above. Thanks

MatthewFlamm · 2024-04-29T18:24:34Z

Hmmmm, IMO any fix we put in here for the testing will be brittle as we will be testing for edge cases in the weather integration handling, rather than nws functionality. I will go back to proposing the fixes in #115857, which adds in the listeners in async_added_to_hass, but also removes the forecast coordinators from the available check. I think if you do this here, you will get the same catch-22 I run into with #115857. I am soon going to test a new version of pynws that will lessen the need for custom availability handling in the nws integration. I'm hoping this simplifies this maintenance headache going forward.

kamiyo · 2024-04-29T18:57:53Z

Yea ok. I think really what hass needs it not only an unavailable state but a stale state.

…

On Mon, Apr 29, 2024, 13:24 MatthewFlamm ***@***.***> wrote: Hmmmm, IMO any fix we put in here for the testing will be brittle as we will be testing for edge cases in the weather integration handling, rather than nws functionality. I will go back to proposing the fixes in #115857 <#115857>, which adds in the listeners in async_added_to_hass, but also removes the forecast coordinators from the available check. I *think* if you do this here, you will get the same catch-22 I run into with #115857 <#115857>. I am soon going to test a new version of pynws that will lessen the need for custom availability handling in the nws integration. I'm hoping this simplifies this maintenance headache going forward. — Reply to this email directly, view it on GitHub <#115836 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWCYT4UGQTC65OOYC3NFJ3Y72F7PAVCNFSM6AAAAABGORCTV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBTGM4DAMBXGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

kamiyo added 2 commits April 19, 2024 04:37

Add missing hourly and twice_daily listeners to NWS

5232d1c

Add hourly and twice_daily coordinator to availability check

2ee71df

home-assistant bot added bugfix cla-signed integration: nws small-pr PRs with less than 30 lines. labels Apr 19, 2024

home-assistant bot added by-code-owner Quality Scale: platinum labels Apr 19, 2024

Merge branch 'dev' into add-coordinator-listeners

d763740

MatthewFlamm mentioned this pull request Apr 19, 2024

Fix nws forecast coordinators and remove legacy forecast handling #115857

Merged

20 tasks

MartinHjelmare changed the title ~~Add coordinator listeners~~ Add nws coordinator listeners Apr 22, 2024

MatthewFlamm reviewed Apr 24, 2024

View reviewed changes

Merge branch 'dev' into add-coordinator-listeners

7d83377

kamiyo added 3 commits April 25, 2024 02:55

Use built-in attributes for coordinators

5615c45

uUpdate nws test to account for increased call count of forecast

a92eef8

Merge branch 'dev' into add-coordinator-listeners

e624074

Merge branch 'dev' into add-coordinator-listeners

fa84239

kamiyo closed this May 4, 2024

github-actions bot locked and limited conversation to collaborators May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nws coordinator listeners #115836

Add nws coordinator listeners #115836

kamiyo commented Apr 19, 2024

home-assistant bot commented Apr 19, 2024

MatthewFlamm commented Apr 19, 2024 •

edited

Loading

MatthewFlamm left a comment

MatthewFlamm Apr 24, 2024

MatthewFlamm Apr 24, 2024

MatthewFlamm commented Apr 24, 2024

kamiyo commented Apr 24, 2024

MatthewFlamm commented Apr 24, 2024

kamiyo commented Apr 26, 2024

MatthewFlamm commented Apr 26, 2024

kamiyo commented Apr 27, 2024

kamiyo commented Apr 29, 2024

MatthewFlamm commented Apr 29, 2024

kamiyo commented Apr 29, 2024 via email

Add nws coordinator listeners #115836

Add nws coordinator listeners #115836

Conversation

kamiyo commented Apr 19, 2024

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Apr 19, 2024

MatthewFlamm commented Apr 19, 2024 • edited Loading

MatthewFlamm left a comment

Choose a reason for hiding this comment

MatthewFlamm Apr 24, 2024

Choose a reason for hiding this comment

MatthewFlamm Apr 24, 2024

Choose a reason for hiding this comment

MatthewFlamm commented Apr 24, 2024

kamiyo commented Apr 24, 2024

MatthewFlamm commented Apr 24, 2024

kamiyo commented Apr 26, 2024

MatthewFlamm commented Apr 26, 2024

kamiyo commented Apr 27, 2024

kamiyo commented Apr 29, 2024

MatthewFlamm commented Apr 29, 2024

kamiyo commented Apr 29, 2024 via email

MatthewFlamm commented Apr 19, 2024 •

edited

Loading