Skip to content

Conversation

@camille-bouvy-frequenz
Copy link
Contributor

Update base client from 0.6.1 to v0.7.0, and upgrade the code accordingly.

@camille-bouvy-frequenz camille-bouvy-frequenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Nov 11, 2024
@camille-bouvy-frequenz camille-bouvy-frequenz requested a review from a team as a code owner November 11, 2024 14:19
@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Nov 11, 2024
@camille-bouvy-frequenz camille-bouvy-frequenz force-pushed the update-base-client branch 2 times, most recently from 560ddd6 to e1f5969 Compare November 11, 2024 16:13
# pylint: disable=no-member
from frequenz.api.electricity_trading.v1 import electricity_trading_pb2
from frequenz.api.electricity_trading.v1.electricity_trading_pb2_grpc import (
ElectricityTradingServiceAsyncStub,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llucax, we can't import the async stub like this, right? Should this only be under if typing.TYPE_CHECKING:?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done here in the example section of the upgrading section of the new base client release. But since it's just an example I don't know if it was tested

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried as well, can't find an easy solution. I guess we need to wait for @llucax

Copy link
Contributor

@Marenz Marenz Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try just importing

from frequenz.api.electricity_trading.v1 import electricity_trading_pb2_grpc

and then just using electricity_trading_pb2_grpc.ElectricityTradingServiceAsyncStub with a global setting to disable the no-member check in pyproject.toml

[tool.pylint.messages_control]
disable = [
  [..]
  # Checked by mypy
  "no-member",
  [..]
]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Marenz for the summary, that should work.

I just updated the release notes (once again) to hopefully provide instructions that finally work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: I wrote electricity_trading_pb2 but meant electricity_trading_pb2_grpc, it is updated in my comment

Copy link
Contributor Author

@camille-bouvy-frequenz camille-bouvy-frequenz Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still get an error from pytest_min and pytest_max:

ERROR AttributeError: module 'frequenz.api.electricity_trading.v1.electricity_trading_pb2_grpc' has no attribute 'ElectricityTradingServiceAsyncStub'

Comment on lines 126 to 132
super().__init__(server_url, ElectricityTradingServiceStub, connect=connect)
super().__init__(server_url, connect=connect)
self._stub = cast(
ElectricityTradingServiceAsyncStub,
ElectricityTradingServiceStub(self.channel),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there's another problem here.

We have to create the _stub when we are connecting. So if connect=False, we can't create the stub immediately, because there's no channel.

And we're going to have to override connect, to call super.connect(), then create the stub.

@llucax this is also not covered in the upgrade notes.

I'll try to make a working patch for this now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new base client's interface feels inconsistent compared to the older one because we have to override connect, but with that, it works for now.

@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Nov 12, 2024
@shsms shsms force-pushed the update-base-client branch from c2440a2 to 3c9cfd9 Compare November 12, 2024 15:49
The user had tried to instantiate the client outside the
asyncio event loop, but it must be created within the loop
to handle asynchronous calls correctly.

This change updates the examples to use async functions
and runs them within an asyncio event loop.

Signed-off-by: Daniel Zullo <[email protected]>
Signed-off-by: camille-bouvy-frequenz <[email protected]>
Also import the `ElectricityTradingServiceAsyncStub` type only when
`TYPE_CHECKING`.

Signed-off-by: Sahas Subramanian <[email protected]>
@github-actions github-actions bot added the part:docs Affects the documentation label Nov 12, 2024
@camille-bouvy-frequenz camille-bouvy-frequenz merged commit 0e6198d into frequenz-floss:v0.x.x Nov 13, 2024
@camille-bouvy-frequenz camille-bouvy-frequenz added this to the v1.0.0 milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants