Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly await for AddInclusionBounds #47

Merged
merged 5 commits into from
May 15, 2024

Conversation

llucax
Copy link
Contributor

@llucax llucax commented May 15, 2024

Without this, the function call will never actually run. I'm not sure if this was working anyways because of some obscure implementation detail in grpcio, but in normal python a not awaited async function should never actually run.

Besides this, for sure errors can't be really caught without the await either.

This was detected while migrating to betterproto, as betterproto has correct type hints.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax requested review from a team as code owners May 15, 2024 10:11
@llucax llucax requested review from thea-leake and shsms May 15, 2024 10:11
@llucax llucax self-assigned this May 15, 2024
@llucax llucax added this to the v0.4.0 milestone May 15, 2024
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:client Affects the client code labels May 15, 2024
@llucax llucax force-pushed the fix-bounds-await branch from 35b7301 to 534e55a Compare May 15, 2024 10:13
llucax added 3 commits May 15, 2024 12:30
Without this, the function call will never actually run. I'm not sure if
this was working anyways because of some obscure implementation detail
in grpcio, but in normal python a not awaited async function should
never actually run.

Besides this, for sure errors can't be really caught without the await
either.

This was detected while migrating to betterproto, as betterproto has
correct type hints.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
All other calls have a timeout, so this one should probably too.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This tests both that the timeout is set and that errors are properly
handled after the `await` bug fix.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax force-pushed the fix-bounds-await branch from 534e55a to 4c94417 Compare May 15, 2024 10:37
@llucax llucax enabled auto-merge May 15, 2024 10:38
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@github-actions github-actions bot added the part:docs Affects the documentation label May 15, 2024
@shsms
Copy link
Contributor

shsms commented May 15, 2024

Btw, the SDK doesn't use the set_bounds method anymore. We use set_power for both EV chargers and PV inverters.

The previous version of the EV charger pool was using this implementation of set_bounds, but that was never deployed.

The older version of the EV charger that was deployed in a demo installation was using an older streaming set_bounds method, so this existing one was never used, and won't be used anymore by the SDK.

@llucax
Copy link
Contributor Author

llucax commented May 15, 2024

OK, I guess that explains why we didn't notice.

@llucax llucax added this pull request to the merge queue May 15, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit e2444b4 May 15, 2024
14 checks passed
@llucax llucax deleted the fix-bounds-await branch May 15, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:client Affects the client code part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants