Bump pysnmp to v7 and brother to v5
#129761
Conversation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
I hope to find time this week to try updating |
|
@nmaggioni Please bump |
|
@bieniu Thanks. I see that you've used the same MIBs caching snippet I did, does it work for you or are the definitions synchronously loaded again on the first use of the engine? |
|
Yeah I copied your solution 😄 I didn't notice any event loop blocking. I checked SNMP sensor and of course Brother. I'll try to test this PR again tonight. |
|
I tested the snmp sensor again and I don't see any event loop blocking. |
|
It seems that only the device_tracker implementation is causing warnings to be logged: to me it looks like the async generator returned by bulk_walk_cmd somehow still triggers blocking I/O detection when it parses the data in the response, but I'm not proficient enough with asyncio to understand why (isn't it already literally async?). Also, I had a dupe assignment in my caching snippet; it did no harm, but you may want to pull that revert too. |
|
I tried to find where this event loop blocking occurs today but I failed. Some asyncio specialist is needed here. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Still valid and needed, but unless logging a blocking I/O warning at boot is acceptable we need help with asyncio troubleshooting. Also rebased on current dev branch. |
|
Maybe try bumping |
|
Could you bump |
MibViewControllerManager.get_mib_view_controller already assigns the cached variable, no need to do that manually. This reverts commit 767d855.
|
Sorry, this slipped through. Anyway, I've bumped the version but the issue is still there: MIBs are getting (re?)loaded when the walker is first called, so HA reports the relative fs calls. I've retraced my steps briefly but I'm still stumped. Since the walker itself is an async iterator it can't be run inside an executor wrapper, but that point is more or less where the code flow moves away from HA and into pysnmp. |
|
@lextm Sorry to tag you directly, maybe you can help here? |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Hi @nmaggioni I managed to get some hardware that allows me to configure SNMP device tracker. I tried today with |
|
Thanks @bieniu for testing. I still get the same warnings though, did you enable the |
Please post a log.
Yes |
|
Pretty much the same warnings as the first comment, so at a first glance the issue is still the MIB table not being preloaded. Logs
Are you testing against a real or a simulated device? I have two physical targets configured, Config
# Loads default set of integrations. Do not remove.
#default_config:
config:
ffmpeg:
# Load frontend themes from the themes folder
frontend:
themes: !include_dir_merge_named themes
automation: !include automations.yaml
script: !include scripts.yaml
scene: !include scenes.yaml
logger:
default: info
logs:
homeassistant.components.cloud: debug
device_tracker:
- platform: snmp
host: <IP>
community: <community>
baseoid: <OID>
interval_seconds: 15
consider_home: 15
new_device_defaults:
track_new_devices: false |
Real device but... my logger was configured for error level 🤦♂️I can see "blocking call" warnings right now. |
These changes should get rid of async warnings Co-authored-by: Maciej Bieniek <[email protected]>
|
Thanks @emontnemery, your patch seems to have done the trick! I've also bumped the pysnmp version again while I was at it; this PR should now finally be ready for merging. |
| self._target = target # pylint: disable=attribute-defined-outside-init | ||
|
|
||
| self.__init__(config) # pylint: disable=unnecessary-dunder-call | ||
| return self |
There was a problem hiding this comment.
Maybe something like this? In this case we can give up calling RuntimeError in the constructor.
| self._target = target # pylint: disable=attribute-defined-outside-init | |
| self.__init__(config) # pylint: disable=unnecessary-dunder-call | |
| return self | |
| instance = cls(config) | |
| instance._target = target | |
| return instance |
And we can define in the constructor self._target type as:
self._target: UdpTransportTarget | Udp6TransportTarget
Proposed change
Spurred by #129685, this PR updates the
pysnmpdependency to its most recent next major version, v7.1.21.I have tested these changes in my setup using the device_tracker platform only, but the synthetic tests for the other platforms (sensor, switch) look okay.
I can't seem to get the MIBs to cache properly, though: I tried to update the old mechanism by looking at pysnmp's own tests but I keep getting a couple of
Detected blocking callwarnings logged when the first SNMP scan starts.Logged warnings
Since thebrothercomponent also depends on pysnmp, @bieniu may want to chime in.brotherupdated to5.0.0.Type of change
Additional information
Checklist
ruff format homeassistant tests)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.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: