Create initial structure #14

Marenz · 2024-02-05T18:05:27Z

pylint still makes some problems, apparently it can't find the generated files?

Marenz · 2024-02-05T18:08:23Z

@llucax is there a trick to make pylint happy or do I need to add exceptions for most of those?

nox > pylint src docs noxfile.py tests
************* Module frequenz.client.dispatch._client
# This one is clear
src/frequenz/client/dispatch/_client.py:100:9: W0511: TODO handle result (fixme)
src/frequenz/client/dispatch/_client.py:11:0: E0611: No name 'Timestamp' in module 'google.protobuf.timestamp_pb2' (no-name-in-module)
src/frequenz/client/dispatch/_client.py:72:28: E1101: Module 'frequenz.api.dispatch.v1.dispatch_pb2' has no 'TimeIntervalFilter' member (no-member)
src/frequenz/client/dispatch/_client.py:83:33: E1101: Module 'frequenz.api.dispatch.v1.dispatch_pb2' has no 'ComponentSelector' member (no-member)
src/frequenz/client/dispatch/_client.py:90:18: E1101: Module 'frequenz.api.dispatch.v1.dispatch_pb2' has no 'DispatchFilter' member (no-member)
src/frequenz/client/dispatch/_client.py:96:18: E1101: Module 'frequenz.api.dispatch.v1.dispatch_pb2' has no 'DispatchListRequest' member (no-member)
************* Module test_dispatch_client
# This one is clear
tests/test_dispatch_client.py:9:4: C0415: Import outside toplevel (frequenz.client.dispatch.Client) (import-outside-toplevel)

llucax · 2024-02-06T08:27:39Z

It is a known bug, we need to put an ignore in the import line:

Module 'google.protobuf.any_pb2' has no 'Any' member pylint-dev/pylint#6281

llucax

Hi, some early feedback. About the SDK thing, if it is only a temporary hack to get things going, I'm good with it!

pyproject.toml

src/frequenz/client/dispatch/_client.py

llucax · 2024-02-06T08:38:04Z

src/frequenz/client/dispatch/_client.py

+        if interval_filter:
+            time_interval = dispatch_pb2.TimeIntervalFilter(
+                start_from=to_timestamp(interval_filter.start_from),
+                start_to=to_timestamp(interval_filter.start_to),
+                end_from=to_timestamp(interval_filter.end_from),
+                end_to=to_timestamp(interval_filter.end_to),
+            )


The idea is that your TimeIntervalFilter wrapper class has from_proto() and to_proto() methods. For now in the SDK we only have from_proto() (example) but I guess it could make sense to have the other too.

That said, to be honest, I don't like this approach, as the idea is to hide the underlying protocol we are using in the client, the idea is that the client user doesn't need to know about protobuf. Since conversion only needs to happen in the client internally, I would prefer an approach where we provide conversion functions separately from the wrapper class, similar to what marshmallow does, or even dataclass when converting to dict or tuple.

Maybe this is something to discuss in the next API meeting (or even SDK meeting, as this is a Python thing more than a API thing).

FYI @frequenz-floss/python-sdk-team

I am not sure for this particular type it's worth having all that, given that it's only used in this function and no-where outside, and the conversion is extremely simple

Right, maybe I picked the wrong example.

tests/test_dispatch_client.py

llucax

Mostly minor things, except for the gRPC mock server which caused us a lot of headaches in the SDK.

.github/workflows/ci.yaml

src/frequenz/client/dispatch/_types.py

llucax · 2024-02-08T12:04:56Z

src/frequenz/client/dispatch/_types.py

+        pb_dispatch.selector.CopyFrom(_component_selector_to_protobuf(self.selector))
+        pb_dispatch.is_active = self.is_active
+        pb_dispatch.is_dry_run = self.is_dry_run
+        # pb_dispatch.payload = self.payload


Why is this commented out?

It caused trouble because the attribute is read-only. Haven'- investigated more

Oh, weird. Maybe add a comment or create an issue about it?

llucax · 2024-02-08T12:06:59Z

tests/test_dispatch_client.py

+@asynccontextmanager
+async def grpc_servicer() -> AsyncGenerator[tuple[aio.Server, str], None]:
+    """Async context manager to setup and teardown a gRPC server for testing."""
+    server = aio.server()
+    dispatch_servicer = DispatchServicer()
+    dispatch_pb2_grpc.add_MicrogridDispatchServiceServicer_to_server(
+        dispatch_servicer, server
+    )
+    port = server.add_insecure_port("localhost:0")
+    await server.start()
+    try:
+        yield server, f"localhost:{port}"
+    finally:
+        await server.stop(None)


I should not use this mock service if it is really listening to a port, we did that in the SDK and it had a lot of issues, among them that multiple tests can be run at the same time. We should mock the server instead at the python level, patching the functions we are calling in our code.

All you need is to mock/patch (in the pyhon sense) this function used by the client: self._stub.ListMicrogridDispatches(request) to return whatever mock data you want to have. We don't need to test the whole gRPC stuff, only our client code.

We could also do a test of the whole thing, but that should be an integration test, not a unit test.

tests/test_dispatch_client.py

llucax · 2024-02-08T12:09:01Z

tests/test_dispatch_types.py

+    for selector in (
+        [1, 2, 3],
+        [10, 20, 30],
+        ComponentCategory.BATTERY,
+        ComponentCategory.GRID,
+        ComponentCategory.METER,
+    ):
+        protobuf = _component_selector_to_protobuf(selector)  # type: ignore
+        assert _component_selector_from_protobuf(protobuf) == selector


Any reason not to use pytest.mark.parametrize for this? It will move some clutter out of the test itself.

Maybe you can borrow some ideas from here if you have more complex tests and you want, for example, to assign names to them, etc.:

https://github.com/frequenz-floss/pymdownx-superfence-filter-lines-python/blob/231d34c4f80ad00785b231d49e28453b5ccfb08e/tests/test_do_validate.py#L33-L127

Tbh, I didn't feel the need to do more complex testing with these methods, they seem simple enough

Sure, you don't have to make it more complicated, but using parametrize() is actually simpler, you don't need to create classes for test cases, that was just a FYI in case it was useful. Just using parametrize is as simple as:

from frequenz.client.common.microgrid.components.components import ComponentCategory from frequenz.client.dispatch._types import ( ComponentSelector, _component_selector_from_protobuf, _component_selector_to_protobuf, ) @pytest.mark.parametrize("selector", [ [1, 2, 3], [10, 20, 30], ComponentCategory.BATTERY, ComponentCategory.GRID, ComponentCategory.METER, ]) def test_component_selector(selector: list[int] | ComponentCategory) -> None: """Test the component selector.""" protobuf = _component_selector_to_protobuf(selector) # type: ignore assert _component_selector_from_protobuf(protobuf) == selector

Another advantage of using parametrize is it creates one test per parameter, so you get exactly which parameter failed and you can execute the test for one particular parameter only, also without it, the test will be aborted after the first failure, while using parametrize will continue testing other parameters (unless the fail early option is used).

If you think updating these tests here now are not worth it, I'm OKish with that, but I strongly suggest using parametrize() in future tests.

llucax · 2024-02-08T12:09:36Z

tests/test_dispatch_types.py

+    from frequenz.client.common.microgrid.components.components import ComponentCategory
+
+    from frequenz.client.dispatch._types import (
+        ComponentSelector,
+        _component_selector_from_protobuf,
+        _component_selector_to_protobuf,
+    )


Any reason for the imports inside the test?

I guess keeping things near where they are used. No deeper reason

If there is no good reason, I would put it on the top-level, which is the most popular style and doesn't require an exception for pylint.

tests/test_dispatch_types.py

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz · 2024-02-14T18:14:17Z

@llucax this is almost ready, some mypy/pylint/flake8 complains, but everything else is working already. Feel free to review already.

llucax

Besides the minor (optional) comments, I'm mostly OK to merge this as is, but there are a few more major points that I think we should change in the future:

Make list async iterable
Make update type safe
Separate public from private classes more clearly (I would put them in different files)
Make sure public classes don't inherit from private classes

PS: I didn't had a look at the tests yet

pyproject.toml

src/frequenz/client/dispatch/_client.py

llucax · 2024-02-15T08:40:32Z

src/frequenz/client/dispatch/_client.py

+        if start_from or start_to or end_from or end_to:
+            time_interval = PBTimeIntervalFilter(
+                start_from=to_timestamp(start_from),
+                start_to=to_timestamp(start_to),
+                end_from=to_timestamp(end_from),
+                end_to=to_timestamp(end_to),
+            )


Just out of curiosity, this is some sort of optimization to send a message as small as possible over the wire, right? But it should be also possible to always create the PB object with all Nones and it will have the same effect?

If so maybe you can add a comment to clarify it, so nobody thinks it is redundant in the future and removes the if.

But it should be also possible to always create the PB object with all Nones and it will have the same effect?

I'd need to research that to be sure, right now, no idea

src/frequenz/client/dispatch/_client.py

llucax · 2024-02-15T09:11:36Z

src/frequenz/client/dispatch/_client.py

+    async def update(
+        self,
+        dispatch_id: int,
+        new_fields: dict[str, Any],


Suggested change

new_fields: dict[str, Any],

**new_fields: Any,

But I would actually list explicitly everything that can be updated here, otherwise we lose type safety and also we allow introducing bugs because of typos (update(start_tim=...) won't be caught by mypy) and the code is quite convoluted with the huge match statement. Actually now that I see it the **new_fields: Any won't work because you use . in keys.

For this PR I'm fine with this but I think we should definitely do it more static and type checked in the future.

the code is quite convoluted with the huge match statement

It would look similar with any other solution as I have to take care / convert every (type of ) member individually.. if it's not a match statement it would be a if para: ... instead for each parameter..

Yeah, is true that you will still check which ones are set to be updated and which are not one by one, but you could at least group RecurrenceRuleUpdate in wrapper, so that can be omitted as a group if nothing has to be changed for example, but the main point here for me is still type safety and being able to get errors at mypytime :D

I though about other options, using TypedDict for example, to avoid the bloat, but all have their issues, I guess this is something we might need to think more about, so I would leave it as in this PR and create an issue about it. I think how we do updates should also be consistent among all API clients, so is something we might want to discuss with the other API teams.

A data class with every type optional as a parameter could be a way.

Yeah, we would need to use a sentinel to indicate if a value was set or not (because you might need to update something from a value to None, so None can't mean "this attribute is not updated") but that still requires you to check one by one every attribute to see if it was overridden or not. dicts are the only ones that have the nice property of encoding the absence of a field natively, so you can just iterate over the existing keys to set the attributes in the message.

I could also use the top suggestion and use _ instead of . as path separator..

For what exactly?

For making **new_fields: Any possible

src/frequenz/client/dispatch/_types.py

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Feb 5, 2024

Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Feb 5, 2024

llucax reviewed Feb 6, 2024

View reviewed changes

Marenz force-pushed the init branch 4 times, most recently from 39333a8 to 1fc6612 Compare February 7, 2024 13:46

Marenz marked this pull request as ready for review February 7, 2024 13:48

Marenz requested a review from a team as a code owner February 7, 2024 13:48

Marenz requested review from ktickner and removed request for a team February 7, 2024 13:48

Marenz self-assigned this Feb 7, 2024

Marenz requested a review from llucax February 7, 2024 13:48

Marenz force-pushed the init branch 5 times, most recently from 23b5783 to 8e72bde Compare February 7, 2024 18:13

llucax reviewed Feb 8, 2024

View reviewed changes

Fix CI cross arch check

1eea5df

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the init branch from 8e72bde to 6a62f87 Compare February 14, 2024 18:13

llucax reviewed Feb 15, 2024

View reviewed changes

Marenz mentioned this pull request Feb 19, 2024

Add a DatetimeRange dataclass to pass pairs from/to #15

Open

Marenz added 2 commits February 20, 2024 16:49

Prepare dependencies for client

72bf6ff

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Add client implementation

e3ea003

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the init branch from 6a62f87 to e3ea003 Compare February 20, 2024 15:49

Marenz requested a review from llucax February 20, 2024 15:50

leandro-lucarella-frequenz approved these changes Feb 21, 2024

View reviewed changes

Marenz merged commit c6a2ee9 into frequenz-floss:v0.x.x Feb 21, 2024
14 checks passed

Marenz deleted the init branch February 21, 2024 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create initial structure #14

Create initial structure #14

Marenz commented Feb 5, 2024

Marenz commented Feb 5, 2024 •

edited

Loading

llucax commented Feb 6, 2024

llucax left a comment

llucax Feb 6, 2024

Marenz Feb 6, 2024

llucax Feb 6, 2024

llucax left a comment

llucax Feb 8, 2024

Marenz Feb 8, 2024

llucax Feb 9, 2024

llucax Feb 8, 2024

llucax Feb 8, 2024

Marenz Feb 8, 2024

llucax Feb 9, 2024

llucax Feb 8, 2024

Marenz Feb 8, 2024

llucax Feb 9, 2024

Marenz commented Feb 14, 2024

llucax left a comment

llucax Feb 15, 2024

Marenz Feb 15, 2024

llucax Feb 15, 2024

Marenz Feb 15, 2024

llucax Feb 15, 2024

llucax Feb 15, 2024

Marenz Feb 19, 2024

llucax Feb 19, 2024

Marenz Feb 20, 2024

leandro-lucarella-frequenz Feb 21, 2024

Marenz Feb 21, 2024

Create initial structure #14

Create initial structure #14

Conversation

Marenz commented Feb 5, 2024

Marenz commented Feb 5, 2024 • edited Loading

llucax commented Feb 6, 2024

llucax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llucax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Feb 14, 2024

llucax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Feb 5, 2024 •

edited

Loading