Refactoring

[abates]

Hello,

I've recently been playing with this package, and ended up doing a ton of refactoring in my fork. I'm wondering if you have any interest in the changes I've made? The changes end up reducing the main code by about 25% and the tests by about 75%. I think the code base is close to functionally equivalent.

If you have no interest in these changes, I totally understand. If this is the case, do you have any issue with me publishing under a different name (something like async-unifi maybe)? I will definitely keep the original commits as well as MIT license and attribution if this is the case.

Thanks for all the work you've put into this pacakge!

[Kane610]

Hello!

I only got to look at it shortly, will look more, in general I'd be happy to make improvements. I welcome contributions.

I'll look a bit more tomorrow. I suggest small PRs moving forward and keeping tests as much as possible intact will keep home assistant integration working, which is my main concern

Lets have a discussion in a couple of days what could be a good starting point

[Kane610]

Hey! I've looked through it and I like a lot of the changes,

Two things I see as important here

1. moving the library over needs to be taken in steps to be able to better grasp each change and properly sync changes with the unifi integration
2. there has been some performance reviews of the library and its important to keep it in a similar state

If you're ok with this we can get going.

/R

[abates]

Hi... This seems reasonable to me. Is there a particular area that you'd like to start?

Regarding performance, I did change from using orjson to Python's builtin json parser. I know that orjson claims to have better performance than the builtin parser, so if there is a performance bottleneck that is something we can look at.

I'm curious in what situations has performance come up? I've used Python's json parser to manage tens of thousands of results in single json documents and the bottleneck is almost always waiting for the response to return from the remote host, not parsing the data within Python.

[thecode]

I'm curious in what situations has performance come up? I've used Python's json parser to manage tens of thousands of results in single json documents and the bottleneck is almost always waiting for the response to return from the remote host, not parsing the data within Python.

Eventually for Home Assistant which is still support running Raspberry PI3, with large number of integrations that use JSON it starts to accumulate.

Anyhow my best suggestion (also with similar on a repo that I maintain) is to start with smaller PRs as long as things are not related and can be done in separate PRs, it helps reviewing faster and if something is still in discussion other PRs can progress if they don't based on each other.

[abates]

@thecode That's fair.. I use Homeassistant on a Pi (although I'm using a 4), so I'm familiar with some of the resource constraints there.

[Kane610]

Regarding performance, I didn't even notice your change for orjson, I don't see a reason for that change though, it's very well used HA as well. Im also not sure about two other changes, renaming controller to client (we already have clients in the library, might be confusing), and moving all web requests into the main class rather than keeping it separate under interfaces. Lets discuss each change as we move along.

I meant more that one of the major changes is that you create new objects more often than before and also go through all values, where as properties are used currently so no parsing of data unnecessarily. Not sure if that would make much difference or not, more that Im pointing it out. Not sure if @bdraco has any input on this suggestion.

If it is possible start with api_handlers or models/api preferably in controllable chunks.

I forgot to ask what triggered your rewrite of the library.

[bdraco]

The JSON overhead was the largest single factor in performance. I do know that I already can't run this on my RPI4b with a large (/21) network without making Home Assistant sluggishly unresponsive in the UI because all the state writes overload the event loop. That's not a problem for a RPI5 or a faster system though, and not a lot of users are going to have a /21. However, will definitely get some complaints if things get slower.

[abates]

Switching back to orjson is easy enough. I was trying to remove as many dependencies as possible, mostly to see what was the bare minimum needed to remain functionally equivalent.

The reason I started hacking away at this was because I wanted to add some additional functionality and I found the existing approach confusing, especially the tests.

I get the feeling that people are happy with how aiounifi is written as-is, and that's fine with me. The changes I want aren't something I care to see implemented in Homeassistant, but in a completely different product that doesn't have to use aiounifi.

[Kane610]

I get the feeling that people are happy with how aiounifi is written as-is, and that's fine with me. The changes I want aren't something I care to see implemented in Homeassistant, but in a completely different product that doesn't have to use aiounifi.

It is up to you, it will be some effort. I am happy going forward improving the library further, I don't think it will ever be perfect, but more eyes on it with different experiences will help make it better

[abates]

So I wasn't volunteering to open PRs, only asking if there was interest in the changes. Since there is no objection, I will continue with the fork and release a different named package on pypi.

Thanks again for building this client in the first place.

[Kane610]

I would have much preferred a collaboration and improving this library than seeing more forks and alternatives out there. More eyes and experiences on one code base would benefit more people. But if thats the way you want it that is of course fine as well.

Maybe i will take inspiration from your changes in the future.

Take care