Fix nws forecast coordinators and remove legacy forecast handling #115857

MatthewFlamm · 2024-04-19T16:38:33Z

Proposed change

This PR adds listeners to the forecast coordinators to fix issue with the forecast updating. In doing so, the NWS handling of retries was discovered to be brittle to changes in core. It was suggested on discord to put the retry behavior upstream into pynws.

This enables further removal of custom code in the integration. Multiple years tests have been pared down now that we do not need to test custom things in the integration.

Release notes for pynws: https://github.com/MatthewFlamm/pynws/releases/tag/v1.7.0

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, Problem with weather update using twice_daily #115288
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-19T16:38:41Z

Hey there @kamiyo, 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-19T16:39:44Z

This is still WIP, but opening early, so that it can be contrasted with #115836.

MatthewFlamm · 2024-04-19T17:36:03Z

tests/components/nws/test_weather.py

@@ -355,7 +322,7 @@ async def test_forecast_service(

    assert instance.update_observation.call_count == 2
    assert instance.update_forecast.call_count == 2
-    assert instance.update_forecast_hourly.call_count == 1
+    assert instance.update_forecast_hourly.call_count == 2


Failure in this test on line above shows that CoordinatorWeatherEntity.forecast_coordinators["twice_daily"] is not called on a time basis, at least in this integration implementation. This PR tries to use the CoordinatorWeatherEntity methods as much as possible here however.

CoordinatorWeatherEntity does not have a coordinator.async_add_listener call in async_added_to_hass.

core/homeassistant/components/weather/__init__.py

Lines 1116 to 1129 in a8025a8

async def async_added_to_hass(self) -> None:

"""When entity is added to hass."""

await super().async_added_to_hass()

self.async_on_remove(partial(self._remove_forecast_listener, "daily"))

self.async_on_remove(partial(self._remove_forecast_listener, "hourly"))

self.async_on_remove(partial(self._remove_forecast_listener, "twice_daily"))

def _remove_forecast_listener(

self, forecast_type: Literal["daily", "hourly", "twice_daily"]

) -> None:

"""Remove weather forecast listener."""

if unsub_fn := self.unsub_forecast[forecast_type]:

unsub_fn()

self.unsub_forecast[forecast_type] = None

It only occurs if a subscription is performed first:

https://github.com/home-assistant/core/blob/a8025a8606fa63be570fb43f0f0643068bef8844/homeassistant/components/weather/__init__.py#L1132C5-L1141C10

coordinator.async_add_listener is now called in async_added_to_hass to fix this.

MatthewFlamm · 2024-04-19T17:37:37Z

homeassistant/components/nws/weather.py


    async def async_update(self) -> None:
        """Update the entity.

        Only used by the generic entity update service.
        """
        await self.coordinator.async_request_refresh()
-        await self.coordinator_forecast_legacy.async_request_refresh()
+        if self.forecast_coordinators["twice_daily"] is not None:


This sort of behavior would be better in CoordinatorWeatherEntity in my opinion.

MatthewFlamm · 2024-04-23T10:52:49Z

@kamiyo this PR goes further than yours by removing unneeded methods, legacy_forecast related, and we don't need the _handle_update_* methods as the upstream library already does the caching. Also the available code no longer should depend on the forecasts as the entity's state no longer has the forecast.

But now the one test for available when the observation coordinator has an error is failing. I cannot figure this out. Did I make some logic error here?

Another thought is that our custom coordinator override of _schedule_refresh could be causing problems.

MatthewFlamm · 2024-04-23T11:18:45Z

These lines are the cause

core/homeassistant/helpers/update_coordinator.py

Lines 406 to 407 in 2977ec4

    
           if not self.last_update_success and not previous_update_success: 
        
               return

Previously, the forecast update would notify the entity to update it's availability. Now it will never update it's availability if it has consecutive failures in a row. Will remove here for feedback.

kamiyo · 2024-04-24T05:04:08Z

@MatthewFlamm Is this change to update_coordinator.py causing the test with the mock update function to fail? Should we update that test?

What was the original issue you were having with available failing? Was it

core/tests/components/nws/test_weather.py

Line 250 in 8a0acf4

assert state.state == STATE_UNAVAILABLE

MatthewFlamm · 2024-04-24T11:30:35Z

@kamiyo, (edited) I had a chat with some core devs on discord on this topic. They did not seem keen to change the core code, but suggested moving the multiple retry behavior inside pynws so that it looks like 1 API call to HA. This will take some time unfortunately. Maybe your PR should move forward in the meantime to fix the behavior, then this one can clean everything up?

kamiyo · 2024-05-01T14:33:27Z

I'll test this locally

MatthewFlamm · 2024-05-01T14:35:49Z

Thanks for testing. This should be mostly done so it is ready for codeowner review.
I want to go over this once more and do more live testing before I officially mark it as "ready" for wider review.

MatthewFlamm · 2024-05-02T01:10:38Z

I have tested locally and it is working well, so I'm opening for full review.

kamiyo · 2024-05-02T03:17:05Z

My local testing is also stable and working.

bdraco

There are unrelated files in this PR

tests/components/geocaching/pycache/init.cpython-312.pyc.139816749228096

homeassistant/components/nws/weather.py

bdraco · 2024-05-02T14:46:12Z

seems to work just fine for me

MatthewFlamm · 2024-05-02T19:05:27Z

There are unrelated files in this PR

tests/components/geocaching/pycache/init.cpython-312.pyc.139816749228096

Was having problems with dependencies and pre-commit mypy and was forced to use git commit -n a few times. This crept in, good catch.

bdraco · 2024-05-02T19:16:33Z

Looks like forecast data is working but everything has an unknown state

MatthewFlamm · 2024-05-02T19:26:44Z

I cannot test live now, but can later. In the meantime, this could be the infamous NWS server temporarily returning junk data problem.

I plan on adding in a retry also when the data is blank, and returning an error if the data continues to be blank after the specified total retry time. [Edit: currently we only retry if there is a server error like 502]. This is for upstream. Captured this thought in MatthewFlamm/pynws#194

MatthewFlamm · 2024-05-02T22:02:28Z

Working locally for me for KHOU

bdraco · 2024-05-02T22:45:23Z

It recovered on its own

bdraco · 2024-05-02T22:46:04Z

PHOG still seems to be flakey but it seems like its the incoming data

bdraco · 2024-05-02T22:50:56Z

I think we can tag this for 2024.5.1 since the removal of the legacy forecast handling shouldn't be considering a breaking change since that was already removed.

Let me know if that sounds right or we should wait for 2024.6.x

MatthewFlamm · 2024-05-02T23:38:43Z

I was under the impression that the legacy forecast handling was totally removed in core, if that is true, then I agree with you that this would be good to land 2024.5.x. There are subtle changes in behavior (availability of the entity, for example), but they are intended to support a bug fix.

MatthewFlamm · 2024-05-03T01:27:18Z

A final live check is that the hourly forecast has been updating nicely, which indicates the problem is indeed fixed. From the same example as I posted above running continuously for a few hours, the hourly forecast is current (as of 9 PM EDT, not CDT). So I'm good with this. Thanks for the help and review!

…15857) Co-authored-by: J. Nick Koston <nick@koston.org>

simplify nws and remove legacy handling

0267de2

home-assistant bot added bugfix cla-signed has-tests integration: nws small-pr PRs with less than 30 lines. by-code-owner Quality Scale: platinum labels Apr 19, 2024

MatthewFlamm mentioned this pull request Apr 19, 2024

Add nws coordinator listeners #115836

Closed

20 tasks

MatthewFlamm commented Apr 19, 2024

View reviewed changes

MatthewFlamm added 2 commits April 20, 2024 07:18

add listener

e9bb5bf

fix available check

c0f29f6

MatthewFlamm and others added 2 commits April 23, 2024 07:20

remove core code preventing updates

b06303d

Merge branch 'dev' into nws-forecast-coord

8a0acf4

MatthewFlamm added 3 commits April 29, 2024 20:23

revert changes to core

3c17d0d

bump pynws

b1ea339

simplify nws coordinators

79018d7

MatthewFlamm marked this pull request as ready for review May 2, 2024 01:10

Merge branch 'dev' into nws-forecast-coord

88b1d2a

bdraco requested changes May 2, 2024

View reviewed changes

bdraco reviewed May 2, 2024

View reviewed changes

homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved

kamiyo suggested changes May 2, 2024

View reviewed changes

homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved

homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved

homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved

MatthewFlamm added 3 commits May 2, 2024 17:37

remove __pycache__ file

44f713d

use walrus for observations

a7abd52

DRY for forecast_coordinators

e6c3309

MatthewFlamm marked this pull request as ready for review May 2, 2024 19:14

home-assistant bot requested review from bdraco and kamiyo May 2, 2024 19:14

bdraco approved these changes May 2, 2024

View reviewed changes

Merge branch 'dev' into nws-forecast-coord

04a4969

bdraco added this to the 2024.5.1 milestone May 2, 2024

kamiyo approved these changes May 3, 2024

View reviewed changes

frenck merged commit 6413376 into home-assistant:dev May 3, 2024
38 checks passed

MatthewFlamm deleted the nws-forecast-coord branch May 3, 2024 11:10

frenck added the cherry-picked label May 3, 2024

frenck pushed a commit that referenced this pull request May 3, 2024

Fix nws forecast coordinators and remove legacy forecast handling (#1…

4e4ac79

…15857) Co-authored-by: J. Nick Koston <nick@koston.org>

frenck mentioned this pull request May 3, 2024

2024.5.1 #116696

Merged

kamiyo mentioned this pull request May 4, 2024

NWS Weather integration not compatible with new get_forecasts service #111146

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nws forecast coordinators and remove legacy forecast handling #115857

Fix nws forecast coordinators and remove legacy forecast handling #115857

MatthewFlamm commented Apr 19, 2024 •

edited

Loading

home-assistant bot commented Apr 19, 2024

MatthewFlamm commented Apr 19, 2024

MatthewFlamm Apr 19, 2024

MatthewFlamm Apr 19, 2024 •

edited

Loading

MatthewFlamm May 2, 2024

MatthewFlamm Apr 19, 2024

MatthewFlamm commented Apr 23, 2024

MatthewFlamm commented Apr 23, 2024

kamiyo commented Apr 24, 2024

MatthewFlamm commented Apr 24, 2024 •

edited

Loading

kamiyo commented May 1, 2024

MatthewFlamm commented May 1, 2024

MatthewFlamm commented May 2, 2024

kamiyo commented May 2, 2024

bdraco left a comment

bdraco commented May 2, 2024

MatthewFlamm commented May 2, 2024

bdraco commented May 2, 2024

MatthewFlamm commented May 2, 2024 •

edited

Loading

MatthewFlamm commented May 2, 2024

bdraco commented May 2, 2024

bdraco commented May 2, 2024

bdraco commented May 2, 2024

MatthewFlamm commented May 2, 2024

MatthewFlamm commented May 3, 2024

	async def async_added_to_hass(self) -> None:
	"""When entity is added to hass."""
	await super().async_added_to_hass()
	self.async_on_remove(partial(self._remove_forecast_listener, "daily"))
	self.async_on_remove(partial(self._remove_forecast_listener, "hourly"))
	self.async_on_remove(partial(self._remove_forecast_listener, "twice_daily"))

	def _remove_forecast_listener(
	self, forecast_type: Literal["daily", "hourly", "twice_daily"]
	) -> None:
	"""Remove weather forecast listener."""
	if unsub_fn := self.unsub_forecast[forecast_type]:
	unsub_fn()
	self.unsub_forecast[forecast_type] = None

Fix nws forecast coordinators and remove legacy forecast handling #115857

Fix nws forecast coordinators and remove legacy forecast handling #115857

Conversation

MatthewFlamm commented Apr 19, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Apr 19, 2024

MatthewFlamm commented Apr 19, 2024

MatthewFlamm Apr 19, 2024

Choose a reason for hiding this comment

MatthewFlamm Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

MatthewFlamm May 2, 2024

Choose a reason for hiding this comment

MatthewFlamm Apr 19, 2024

Choose a reason for hiding this comment

MatthewFlamm commented Apr 23, 2024

MatthewFlamm commented Apr 23, 2024

kamiyo commented Apr 24, 2024

MatthewFlamm commented Apr 24, 2024 • edited Loading

kamiyo commented May 1, 2024

MatthewFlamm commented May 1, 2024

MatthewFlamm commented May 2, 2024

kamiyo commented May 2, 2024

bdraco left a comment

Choose a reason for hiding this comment

bdraco commented May 2, 2024

MatthewFlamm commented May 2, 2024

bdraco commented May 2, 2024

MatthewFlamm commented May 2, 2024 • edited Loading

MatthewFlamm commented May 2, 2024

bdraco commented May 2, 2024

bdraco commented May 2, 2024

bdraco commented May 2, 2024

MatthewFlamm commented May 2, 2024

MatthewFlamm commented May 3, 2024

MatthewFlamm commented Apr 19, 2024 •

edited

Loading

MatthewFlamm Apr 19, 2024 •

edited

Loading

MatthewFlamm commented Apr 24, 2024 •

edited

Loading

MatthewFlamm commented May 2, 2024 •

edited

Loading