Any reason why some of the new routes are async and others not?

I think we should keep everything non-async if possible since we are not using the async version of sqlalchemy. This means that any query to the database will actually be blocking.

That being said I'm not sure of the implications for httpx.

The reason we made some of these new routes async is ultimately because of this function:
https://github.com/openclimatefix/pv-site-api/pull/87/files#diff-d682b0ba7c0698e66b6ea6e320db5feb17c1460dbbe67aa2852801b0867f311fR16-R26
(get_inverters_list)

The function needs to make several requests to Enode, one for each of the user's inverters to get detailed inverter data. For performance reasons, we choose to make the requests in parallel, using httpx's AsyncClient and aggregating the results with asyncio. Given we use an AsyncClient instance and we await the asyncio call, we must make the get_inverters_list function async. In turn, we then must make all routes that call get_inverters_list async too.

You're right that the current queries will also block the server in these async routes, which is my mistake, as I thought FastAPI would run all routes in an external thread pool no matter what. Given that the await on get_inverters_list depends on the database queries in these async routes, I think setting up the SQL queries as path dependencies rather than in the route body will prevent the blocking. It seems like any non async dependency will run in an external thread pool even if the path function itself is async docs.

I'm now realizing the Auth path dependency might actually be a good place to grab the client from the database... the way you've implemented it now is quite abstract, so maybe it makes more sense to create a new dependency called get_client, which depends on Auth, and grabs the client (for now, just returns the first one), from the Auth state.

Then, each route which needs the client can just depend on that, and if a route doesn't need the client, can depend on Auth. Or, they can all just use get_client since I'm not sure when a route body would use the decoded JWT payload by itself?

In any case, this fixes our blocking problem by putting the query for the client in a non async path dependency.

I agree with you that if a route body doesn't call the DB (which might be possible here if the information about what is the user's client is coming from Auth0), then we can use async.

This might be the case in this function but it's not the case for all the other async functions in this PR.

Any reason why we can't simply use httpx's httpx.Client() instead of the AsyncClient? It would be simpler and harder to shoot ourselves in the foot in our otherwise non-async setup.

This might be the case in this function but it's not the case for all the other async functions in this PR.

Yes, so the other async functions which make other DB accesses will have to move those accesses to a path dependency as well, one which depends on auth/client.

The reason we can't use httpx.Client() is because it blocks within the thread the route is running on when making requests, and since we're making several independent requests to Enode, we would rather make those requests in parallel, which is only possible with httpx using httpx.AsyncClient().

OK I see your point.

If those really need to run in parallel, the alternative (without async) would be to use threads. Given that we not otherwise using async in the API at the moment, I think that would be the cleaner solution.

This brought to mind a more general question: why do we need to go through the (OCF) API to get the Enode inverter data? Can't we call Enode's API directly from the frontend? The only thing that really needs to get to our backend is the inverter id. In other words, could we have this:

GET /sites/{site_id}/inverters => Returns the list of inverter ids for given site
PUT /sites/{site_id}/inverters => Set the inverter id for given site

And that's it. The frontend would then call Enode with those ids if it needs the full inverter data.

Sounds good, I've also got an implementation of the non-blocking (via path dependencies) database accesses with the async routes, that I'll push in a second. We can definitely switch this to threads though.

Regarding accessing Enode's API directly from the frontend, since their API requires Enode secrets, and has no access control (any one with a token can access all users), it wouldn't be feasible. This would make things significantly simpler though.

Sorry for taking so long on the above message @simlmx, it took me a while to figure out a bug with the way I've set it up and the new cache system, and for now I've temporarily ignored the issue by just ignoring the auth dependency in the cache key. This is not mergable at this point since the auth dependency must be in the cache key, but I still need to add a JSON encoder for the ClientSQL model, or something similar that won't require a pv-site-datamodel change ideally... all in all the code I just pushed is still a work in progress, but hopefully you can see the idea.

This current code does fix the server blocking issue by putting DB accesses in the async def routes into a dependency.

Let me know if you think this general idea works, or if switching to threads is the better option. My only concern with a thread based solution is it will involve additional complexity to do with creating the several threads and waiting on them.

Hi @AndrewLester

Whats the status on this? Is the auth cache key still an issue?
The requests to ENODE to get the inverter data? How long does each on roughly take?

The auth cache key is no longer an issue since I serialize the current request's corresponding client, and use that as part of the key here:

However, since we currently still get the first client in the database for every request, every request that is authenticated will share the same cache for the time being. This doesn't change behavior since the endpoints used the same client anyway.

The requests to Enode to get inverter data take on average 560ms in the sandbox API and, although I can't test the actual inverter endpoints with production since we don't have any users linked there at the moment, the endpoints take around 550ms before returning nothing.

This is on the longer side, but given that most users we'll work with will only have 1-2 inverters, we'd only be looking at a 2x slowdown at most, if were to make these requests in series.