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

Add hub- and device-info for tellduslive #19180

Merged
merged 14 commits into from Dec 23, 2018

Conversation

Projects
None yet
5 participants
@fredrike
Copy link
Contributor

fredrike commented Dec 11, 2018

Description:

Add hub- and device-info for tellduslive.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

@fredrike fredrike force-pushed the fredrike:tellduslive-device_info branch from 4a1b70b to 57268c3 Dec 12, 2018

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

I've analyzed the tellduslive library, and found another place where I/O is done that affects us. It's when the LocalAPISession is created with a token, which happens in our async_create_entry when we provide a host upon instantiating the tellduslive Session.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 14, 2018

We also need to use the executor on lines 55 and 185.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Ok, I think I'm done for this round.

fredrike added some commits Dec 11, 2018

async_get_hubs
Co-Authored-By: fredrike <fredrik.e@gmail.com>

@fredrike fredrike force-pushed the fredrike:tellduslive-device_info branch from 33082a3 to 7e58ba7 Dec 18, 2018

@fredrike fredrike force-pushed the fredrike:tellduslive-device_info branch from 18f06f7 to 54731fa Dec 19, 2018

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Can be merged when build passes.

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 19, 2018

Don't merge quite yet, we need to handle this in a good way:

2018-12-19 17:11:57 WARNING (SyncWorker_1) [urllib3.connectionpool] Connection pool is full, discarding connection: 192.168.1.10
2018-12-19 17:11:57 WARNING (SyncWorker_3) [tellduslive] Failed request: HTTPConnectionPool(host='192.168.1.10', port=80): Read timed out. (read timeout=10)
2018-12-19 17:11:57 WARNING (MainThread) [homeassistant.components.tellduslive.entry] None

fredrike added some commits Dec 19, 2018

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 20, 2018

@MartinHjelmare, What would be reasonable values for pool_connections=10, pool_maxsize=10 to HTTPAdapter? Or should we use pool_block (pool_block – Whether the connection pool should block for connections.)

http://docs.python-requests.org/en/latest/api/#requests.adapters.HTTPAdapter.init_poolmanager

block – Block when no free connections are available.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

I'm not sure. Are we getting these errors specifically with this PR? Then I'm thinking we're doing something wrong, and should try to figure that out. Maybe the HTTPAdapter settings isn't the solution?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

Here's a PR where a similar problem was solved for influxdb component.

#12182

Maybe the problem is that we're doing small work with requests in many different threads, and should try to group more requests/work in the same thread?

A better solution would probably be to move everything to asyncio/aiohttp. But that requires a new library or a major refactor to the tellduslive library.

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 20, 2018

This line to the tellduslive library solves the pool issue (tested on both the cloud and local endpoints):

self.mount('http://', requests.adapters.HTTPAdapter(pool_block=True))

It does what you are suggesting, but on the lower level (waits until a pool is available).

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

Ok, but it's not good to block a thread in the home assistant thread pool. That risks exhausting the pool. It would be good to try to group more requests in the same thread.

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 20, 2018

Ok, but it's not good to block a thread in the home assistant thread pool. That risks exhausting the pool. It would be good to try to group more requests in the same thread.

Could we just increase the pool_maxsize then? I don't feel like I have the time to refactor either the lib or the integration.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

No, we should fix the problem at the root. If we're doing things the wrong way, we should change the way we're doing them. Not keep doing it wrong and try to handle the fallout of that.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

Where are we possibly doing more than one request at a time? If we can make sure to line the requests up, the problem should go away, right? This is one place where we could wait for the request to finish: #19180 (comment).

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 20, 2018

@MartinHjelmare

I've made some tests:

  • Current implementation (yields errors and missing entities) gather: 10.14268684387207 s
  • Current implementation with pool_block gather: 1.1395719051361084 s
  • await for each discover 1.0583820343017578 s
for d_id in new_devices:
    await self._discover(d_id)
  • await for discover split in chunks of 10 1.065885066986084 s
for i in range(0, len(new_devices), 10):
    await asyncio.gather(*[self._discover(d_id) for d_id in list(new_devices)[i:i + 10]])

So I guess waiting for the discover could be ok :).

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 20, 2018

Can be merged when build passes.

@cgarwood cgarwood merged commit 0b84eef into home-assistant:dev Dec 23, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on tellduslive-device_info at 93.055%
Details

@wafflebot wafflebot bot removed the in progress label Dec 23, 2018

dshokouhi added a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018

Add hub- and device-info for tellduslive (home-assistant#19180)
* add hub- and device-info

* attempt to make I/O outside event loop

* add_executer_job

* coroutines

* async_get_hubs

Co-Authored-By: fredrike <fredrik.e@gmail.com>

* asyncio fixes

* do device_info IO when device is discovered

* it's called async_add_executor_job

* nicer unique_id

* add comment

* it's called `async_add_executor_job`

* hub always contains 'name'

* await each new device

* add comment to why gather is bad

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 25, 2018

Merge branch 'dev' into current
* dev: (27 commits)
  Add device_id configuration option to Bluetooth tracker (home-assistant#18539)
  Add homematicip cloud full flush measuring switch (home-assistant#19247)
  Added support for triggered state on NX584 alarm. (home-assistant#19524)
  Add HomematicIP SMI55 device (home-assistant#19400)
  Clean up homematicip cloud (home-assistant#19481)
  Improve Lutron RadioRA2 support, adding switches and scenes (home-assistant#18330)
  Remove global from ZHA application controller (home-assistant#19557)
  Update homekit controller to homekit==0.12.0 (home-assistant#19549)
  Improve handling of MQTT light discovery (home-assistant#19436)
  Fix support for base topic for empty values in MQTT discovery msg (home-assistant#19501)
  Configure ZHA entity on new ZHA device join (home-assistant#19470)
  Add hub- and device-info for tellduslive (home-assistant#19180)
  Fix issues in ZHA light (home-assistant#19368)
  Updated to support per device find iphone sound. (home-assistant#19535)
  Change ISY binary_sensor subnode to hex (home-assistant#19471)
  increase robustness, when something upstream fails (home-assistant#19493)
  Support ZHA light turn_off transition (home-assistant#19531)
  Pywemo version bump (home-assistant#19538)
  Make ZHA entities non-polled by default (home-assistant#19536)
  Add ZHA occupancy sensor (home-assistant#19365)
  ...

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment