Don't sleep on PowerDistributor startup #971
Conversation
This is done by patching the wait time for component data in the battery pool to a lower value. Signed-off-by: Sahas Subramanian <[email protected]>
A function's existence was being checked rather than its (boolean) result. This wasn't an issue in practice, because ever since we've had the PowerManager, the PowerDistributor has not received requests when there's no data. Signed-off-by: Sahas Subramanian <[email protected]>
This is because the initial wait time is going to be removed from the power distributor. Signed-off-by: Sahas Subramanian <[email protected]>
The power distributor is no longer controlled directly by the users, but instead gets requests only from the power manager. So if will not receive requests until there's data. But if it receives requests when there's no data, it will return an `Error` response, saying there's no data. Signed-off-by: Sahas Subramanian <[email protected]>
And instead sleep for the same amount of time, before sending requests to the power distributor. This sleep is needed only in the direct tests of the PowerDistributor. In the `*_pool_control_methods` tests, PowerDistributor receives requests from the PowerManager, which would happen only when there are data, so additional sleep is not required in those tests. Signed-off-by: Sahas Subramanian <[email protected]>
github-actions
bot
added
part:tests
shsms
added
the
cmd:skip-release-notes
|
Only internal changes, so no release notes necessary. |
|
The CI is failing, not sure why suddenly a lot of cross referencing can't be found, but I guess it might be another ripple effect of mkdocstrings/griffe#294. Will do a quick check to see if downgrading the dependency fixes it. |
| mocker.patch.object( | ||
| timeseries.battery_pool._methods, # pylint: disable=protected-access | ||
| "WAIT_FOR_COMPONENT_DATA_SEC", | ||
| 0.1, | ||
| ) |
There was a problem hiding this comment.
Why do we still have some other waiting for data as a simple sleep? Shouldn't that wait be removed too?
There was a problem hiding this comment.
This is for the metric streaming in the battery pool. It won't stream any metrics until it has data for all the batteries, etc. So we don't send a new value for capacity or soc as we get data from more and more batteries.
This is independent of the power distributor.
| ): | ||
| await asyncio.sleep(0.1) # wait for actor to collect data |
There was a problem hiding this comment.
Is this PR just moving the issue elsewhere instead? Or is this synchronization only needed for the tests but when actually used one shouldn't care when there is data and when there isn't?
In any case, having tests depend on random sleep makes me sad (as I see the flakiness coming to bite us eventually). 😢 😞
If we can really really get rid of the need for synchronization, then I completely agree we shouldn't provide any way to synchronize, but otherwise I think having a way to explicitly synchronize that is it not a flaky sleep it is a better approach. In terms of performance, even if it is a hot path, it is just one extra boolean evaluation and branch, which modern CPUs probably won´t even care about because they will do branch prediction so the performance might not even be affected at all after the initialization.
There was a problem hiding this comment.
As I mentioned in the commit message, these are needed only in tests where we control the PowerDistributor directly.
And they are needed only for those tests that need component data, so that PowerDistributor can return Success. Else it will return Error("No data"), but that's not the case we're testing.
It is only a testing problem, not with behaviour in production.
There was a problem hiding this comment.
OK, I'm still not happy about potential flaky tests, but this is still an improvement over what we had before, so approving 👍
Fixed. |
bfc8de2
The PowerDistributor used to sleep for 2 seconds on startup, before
processing requests, to wait for data. This feature is no longer
being used, because it won't receive requests from the PowerManager
unless there's data. This PR removes it.