Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properties that do I/O don't do well with asyncio #3

Open
balloob opened this issue Aug 24, 2019 · 6 comments
Open

Properties that do I/O don't do well with asyncio #3

balloob opened this issue Aug 24, 2019 · 6 comments

Comments

@balloob
Copy link
Contributor

balloob commented Aug 24, 2019

This lib does I/O when a property is set.

# This makes network calls
device.operationMode = "heat"

This makes it difficult to use in asyncio, because now all code will need to be wrapped in synchronous functions. It also makes it risky that if a developer does not pay attention, they end up doing I/O inside the event loop.

Using functions is preferred

device.set_operation_mode("heat")
@timmo001
Copy link

timmo001 commented Apr 30, 2020

I've tried (in my own fork using the git dependency method) changing any @property.setter functions to a standard function with set_ as a prefix but am still running into the new I/O warning on HA.

What else causes I/O inside the event loop apart from the above? Is it the _get function for example as it does a call to the lyric API? How do we resolve this?

Line in question is a call to locations

@balloob
Copy link
Contributor Author

balloob commented Apr 30, 2020

Ideally also the getters need to be removed because they are also doing I/O, like locations: https://github.com/bramkragten/python-lyric/blob/master/lyric/__init__.py#L1169-L1177

I think that the best approach is to remove all caching and helper functions like self._location which expect to be able to iterate over cached results.

Let the lib be responsible for interacting with the API, a caching layer can be build on top by the consumer.

@ctrl50
Copy link

ctrl50 commented May 19, 2020

Hey, Im using the timmo001 fork from here:
https://github.com/home-assistant/core/tree/92082b687a46c17a9a94a598b245728a8832b179/homeassistant/components/lyric
here is some i/o errors incase it helps at all..

Appreciate all the work Timmo. Its to bad there are so many non-working versions of the lyric thermostat. its such a popular unit.. hope to see a working version sometime in the future...

doing I/O at custom_components/lyric/climate.py, line 223: self.device.temperatureSetpoint = temp
doing I/O at custom_components/lyric/climate.py, line 232: self.device.thermostatSetpointStatus = preset_mode

doing I/O at custom_components/lyric/climate.py, line 241: self._humidity = self.device.indoorHumidity
ing I/O at custom_components/lyric/sensor.py, line 122: state = getattr(self.device, self.key)

hope it helps.

@jasondefuria
Copy link

So is it worth stripping out and making a separate helpers.py file in this lib, or remove completely and have it be defined in custom_component/integration?

@bramkragten
Copy link
Owner

We should use the DataUpdateCoordinator of Home Assistant https://developers.home-assistant.io/docs/integration_fetching_data#coordinated-single-api-poll-for-data-for-all-entities in the case of the consumer being Home Assistant.

@jasondefuria
Copy link

@bramkragten Okay, reading that, in my limited understanding at this point- for my lyric custom_component, I should import DataUpdateCoordinator and make all those _get to instead be invoked by DataUpdateCoordinator

New at this and learning!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants