Pass through parameters to client#164
Conversation
Marenz
commented
Jun 12, 2025
- Pass through timeout arguments for the client
- Harden potentially flakey test
- Update release notes
There was a problem hiding this comment.
Pull Request Overview
This PR enables configurable timeouts for the dispatch client, strengthens a flaky test by adding a missing parameter, and updates release notes to reflect the new options.
- Pass
call_timeoutandstream_timeoutthrough toDispatchApiClient - Add
recurrenceparameter in the dispatch schedule test - Document new timeout parameters in release notes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_frequenz_dispatch.py | Added recurrence=RecurrenceRule() argument in test_dispatch_schedule |
| src/frequenz/dispatch/_dispatcher.py | Extended __init__ to accept call_timeout and stream_timeout and pass them to the client |
| RELEASE_NOTES.md | Added entries for the call_timeout and stream_timeout parameters |
Comments suppressed due to low confidence (2)
tests/test_frequenz_dispatch.py:361
- The new
recurrenceparameter is passed in the test, but there are no assertions verifying its effect; consider adding assertions to cover recurrence behavior in dispatch scheduling.
recurrence=RecurrenceRule(),
src/frequenz/dispatch/_dispatcher.py:201
- [nitpick] Consider refactoring the constructor to group related parameters (e.g., timeouts) into a config or options object instead of disabling the lint rule on too many arguments.
# pylint: disable-next=too-many-arguments
RELEASE_NOTES.md
Outdated
| ## New Features | ||
|
|
||
| <!-- Here goes the main new features and examples or instructions on how to use them --> | ||
| * The dispatcher offers two new parameters to control the clients call and stream timeout: |
There was a problem hiding this comment.
Missing apostrophe in "clients call"; it should read "client's call" to indicate possessive form.
| * The dispatcher offers two new parameters to control the clients call and stream timeout: | |
| * The dispatcher offers two new parameters to control the client's call and stream timeout: |
llucax
left a comment
There was a problem hiding this comment.
What copilot said, other than that, LGTM.
| def __init__( | ||
| self, | ||
| *, | ||
| microgrid_id: int, |
There was a problem hiding this comment.
Nothing to do with this PR but I wonder if there is a reason why you need to pass the microgrid_id when creating the client. For edge clients this makes sense, but for cloud clients? You need to create one client per microgrid? This feels weird.
There was a problem hiding this comment.
.. this is an edge client?!
There was a problem hiding this comment.
And, well. We need to know the id we want dispatches for, that's why we need the microgrid id.
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
..by making sure it a dispatch expected to end will never repeat. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>