sunrise_time / sunset_time stopped working

[aardal]

After updating it gives the following error when using sunrise_time and sunset_time: TypeError: async_add_executor_job() got an unexpected keyword argument 'hour'

The error disappears when removing the corresponding two lines in the config.

Tips to solve this is highly appreciated.

error.txt config_1.txt

[D-side]

I kept rolling back until it started working again, stopped at 2.0.6, which works fine. This is not a long-term fix by any means, but perhaps a clue.

[YahyaAlfayad]

I am having the same issue. My config is as follows:

circadian_lighting:
  sunrise_time: "05:30:00"
  sunset_time: "20:30:00"

The issue started after updating.

[claytonjn]

Just want to acknowledge this and mention that I have a few projects going on right now but I'll try to fix this as soon as possible.

[D-side]

Welp, after the release of HA 2022.9 using 2.0.6 is no longer an option due to an API update that only 2.1.2 is compatible with. So the fix to cope for now is, as stated by the OP:

removing the corresponding two lines in the config.

---

Might be a decent time to learn Python! Here's what I've found so far:

• Since 2.1.0-beta the integration made a lot of methods asynchronous (if I'm interpreting this right) and replaced a plain call to date.replace with async_add_executor_job that should've called the same method asynchronously with the same arguments
• The async_add_executor_job, as it exists right now, does not accept any keyword arguments, and this blows up on the above call that has them

So — since 2.1.0-beta manually specifying sunset/sunrise times should've been completely broken. I think that adds up with what I've experienced 🤔

Replacing components of a date doesn't sound like something that should require any asynchrony, as it's an operation entirely for a tiny amount of CPU and RAM, with no I/O. So the fix, I suppose, is to turn that particular bit back to synchronous?

I'll try to come up with a setup to test this and will submit a PR if I succeed.

[claytonjn]

Replacing components of a date doesn't sound like something that should require any asynchrony, as it's an operation entirely for a tiny amount of CPU and RAM, with no I/O. So the fix, I suppose, is to turn that particular bit back to synchronous?

Let me preface this by saying that I am experienced with async systems but am very new to writing async code so I could be very wrong here...but my understanding is that code should be either async or not, mixing async and non-async code can cause problems. async_add_executor_job essentially makes the synchronous code safe to call from an asynchronous process. I haven't had time to look at this yet but it was obviously just an oversight on my part (I don't personally use the set times in config and have relied on the beta releases to uncover bugs rather than implementing proper integration testing.) I'm pretty sure there's a way to wrap the call in async_add_executor_job and pass a parameter but I don't remember off the top of my head.

Please feel free to dive into this if you're so motivated! There's a helpful guide for async with HA here: https://developers.home-assistant.io/docs/asyncio_working_with_async/

[YahyaAlfayad]

Changing

async def _async_replace_time(self, date, key):
        other_date = self._manual_sunrise if key == "sunrise" else self._manual_sunset
        return await self.hass.async_add_executor_job(date.replace,
            hour=other_date.hour,
            minute=other_date.minute,
            second=other_date.second,
            microsecond=other_date.microsecond,
        )

To

async def _async_replace_time(self, date, key):
        other_date = self._manual_sunrise if key == "sunrise" else self._manual_sunset
        return date.replace(
            hour=other_date.hour,
            minute=other_date.minute,
            second=other_date.second,
            microsecond=other_date.microsecond
        )

Fixed everything for me. I agree with @D-side . I personally think scheduling an asynchronous call for date.replace is an overkill and has too much performance overhead compared to the simplicity of the function logic itself.

[D-side]

I've messed with asynchrony a little bit in JavaScript and Clojure (core.async) and my understanding is that declaring a function as async modifies the way it returns its result in such a way that producing said result may take a while — returning a promise of a value rather than a value itself. await can resolve a promise, but it can only be used in async functions. In synchronous code to retrieve a value from a promise you may have to do a blocking wait (I'm not even sure how) — which is usually undesirable.

To achieve this the interpreter splits up the function by awaits and chains the pieces together as promises in the same order — every piece synchronous by itself, but with potential pauses in between. Thus, if there are no awaits in the function body, splitting produces only one piece with the whole function body, the result of which is wrapped in a promise. It's perfectly valid.

But since outside of that seemingly unnecessary await that function doesn't await anything else, nor does it implement any interface where alternate asynchronous implementations may exist, it can probably be safely deasynced. I didn't go that far in my fix though.

[nebbishhacker]

my understanding is that code should be either async or not, mixing async and non-async code can cause problems.

Calling sync code from async code generally only causes problems if the sync code blocks, which is to say it has to wait for something to complete that could take a non-trivial amount of time from the computer's perspective. I/O operations and intense cpu-bound tasks are generally considered to be blocking operations, but simple things like manipulating a datetime object are not.

[D-side]

Hotfix available in #206, it makes the feature work again, and is short enough to easily apply manually if needed. A slightly better stopgap measure for people who aren't happy about their local daylight schedule 🙂

The update tools (HACS?) should happily overwrite it once a new update with this fix rolls out, so should be a smooth transition.

[aardal]

To everyone involved, thanks for the temporary fix, Appreciated!

[claytonjn]

Merged #206 and created release 2.1.3-beta. If there are no complaints after a couple days I'll create a 2.1.3 release.

[claytonjn]

Released 2.1.3 with this fix, thanks again for the contributions everyone!