kamiyo commented 4 weeks ago

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
[x] 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 #115769, #111146, #115433
This PR is related to issue:
Link to documentation pull request:

Checklist

[x] The code change is tested and works locally.
[x] Local tests pass. Your PR cannot be merged unless tests pass
[x] There is no commented out code in this PR.
[x] I have followed the development checklist
[x] I have followed the perfect PR recommendations
[x] The code has been formatted using Ruff (ruff format homeassistant tests)
[x] 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[bot] commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

kamiyo commented 3 weeks ago

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

MatthewFlamm commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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

MatthewFlamm commented 2 weeks ago

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 https://github.com/home-assistant/core/pull/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 commented 2 weeks ago

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 https://github.com/home-assistant/core/pull/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 https://github.com/home-assistant/core/pull/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 https://github.com/home-assistant/core/pull/115836#issuecomment-2083380072, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWCYT4UGQTC65OOYC3NFJ3Y72F7PAVCNFSM6AAAAABGORCTV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBTGM4DAMBXGI . You are receiving this because you authored the thread.Message ID: @.***>

home-assistant / core

Add nws coordinator listeners #115836

Proposed change

Type of change

Additional information

Checklist

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.