Add contains function to Bounds and SystemBounds

[jack-herrmann]

No description provided.

[jack-herrmann]

This is for Issue #950.

[shsms]

could you try: bounds_none: Bounds[Power | None] = Bounds(lower=None, upper=None)

[llucax]

This was a long review, in the middle of testing my proposed changes, I bumped into a (unrelated) race condition in the tests, and I derailed a lot trying to find the issue. Now I think I did, and I realized I completely abandoned the review, so resuming it now 😆

I didn't check @sahas-subramanian-frequenz comments in depth yet, I hope we don't contradict each other 😬

In case it's useful, here is a full diff of my proposed changes that are supposed to be working:

Click to expand

diff --git a/src/frequenz/sdk/timeseries/_base_types.py b/src/frequenz/sdk/timeseries/_base_types.py
index cce408b7..01a36313 100644
--- a/src/frequenz/sdk/timeseries/_base_types.py
+++ b/src/frequenz/sdk/timeseries/_base_types.py
@@ -8,7 +8,7 @@ import functools
 from collections.abc import Callable, Iterator
 from dataclasses import dataclass
 from datetime import datetime, timezone
-from typing import Any, Generic, Protocol, Self, TypeVar, overload, runtime_checkable
+from typing import Any, Generic, Protocol, Self, TypeVar, cast, overload
 
 from ._quantities import Power, QuantityT
 
@@ -134,7 +134,6 @@ class Sample3Phase(Generic[QuantityT]):
         )
 
 
-@runtime_checkable
 class Comparable(Protocol):
     """
     A protocol that requires the implementation of comparison methods.
@@ -167,7 +166,7 @@ class Comparable(Protocol):
         """
 
 
-_T = TypeVar("_T")
+_T = TypeVar("_T", bound=Comparable | None)
 
 
 @dataclass(frozen=True)
@@ -193,15 +192,19 @@ class Bounds(Generic[_T]):
         if item is None:
             return False
 
-        assert isinstance(item, Comparable)
-
-        if self.lower is not None and self.upper is not None:
-            return self.lower <= item <= self.upper
-        if self.lower is not None:
-            return self.lower <= item
-        if self.upper is not None:
+        if self.lower is None and self.upper is None:
+            return True
+        if self.lower is None:
             return item <= self.upper
-        return False
+        if self.upper is None:
+            return self.lower <= item
+        # mypy seems to get confused here, not being able to narrow upper and lower to
+        # just Comparable, complaining with:
+        #   error: Unsupported left operand type for <= (some union)
+        # But we know if they are not None, they should be Comparable, and actually mypy
+        # is being able to figure it out in the lines above, just not in this one, so it
+        # should be safe to cast.
+        return cast(Comparable, self.lower) <= item <= cast(Comparable, self.upper)
 
 
 @dataclass(frozen=True, kw_only=True)
diff --git a/src/frequenz/sdk/timeseries/ev_charger_pool/_set_current_bounds.py b/src/frequenz/sdk/timeseries/ev_charger_pool/_set_current_bounds.py
index faefff5e..81bd5463 100644
--- a/src/frequenz/sdk/timeseries/ev_charger_pool/_set_current_bounds.py
+++ b/src/frequenz/sdk/timeseries/ev_charger_pool/_set_current_bounds.py
@@ -99,8 +99,10 @@ class BoundsSetter:
             _logger.error(err)
             raise RuntimeError(err)
 
+        meter_id = next(iter(meters)).component_id
         self._meter_data_cache = LatestValueCache(
-            await api_client.meter_data(next(iter(meters)).component_id)
+            await api_client.meter_data(meter_id),
+            unique_id=f"{type(self).__name__}«{hex(id(self))}»:meter«{meter_id}»",
         )
         latest_bound: dict[int, ComponentCurrentLimit] = {}
 

[llucax]

I would also add some tests for corner cases, like using plain ints, using an empty range, like Bounds(lower=0, upper=0), using floats, negative numbers, and trying Bounds(lower=10, upper=-10), which is actually not enforced now, not related to this PR but it could be a nice addition to validate the arguments in __post_init__().

I think also adding kw_only to the @dataclass(froze=True, kw_only=True) for both bounds classes would be a great addition too, as it would be very confusing if users use Bounds(10, 14); which is which?

[llucax]

I reopened this because the suggested changes were not made. Even if the extra validation is not added, at least having tests for this would be good:

I would also add some tests for corner cases, like using plain ints, using an empty range, like Bounds(lower=0, upper=0)

If you think this should not be done, it would be good to give a reason.

[jack-herrmann]

This should address all the suggested changes.
I will add further tests through a new commit later.

[llucax]

LGTM, made just a couple of optional comments and I'll wait for the extra tests before approving.

This also needs release notes (add a note in New Features about Bounds and SystemBounds supporting in.

[llucax]

Tests are failing for an unrelated regression (in griffe) :(

I would wait a bit before pinning griffe to an old version because the author is super responsive, so maybe there is a fix soon.

@jh2007github BTW the PR is not based on the current v1.x.x branch, can you rebase it?

[llucax]

I have only one comment about pending stuff from last review. There is other small stuff, like using Self instead of Any in Comparable, but I think we can leave it for a future PR, as this PR is already functional, improvements are optional. Specially since I plan to move this code to the new core repo.

Just as a reminder, a few general improvements that could be made to Bounds:

• frequenz-floss/frequenz-core-python#11
• frequenz-floss/frequenz-core-python#12
• frequenz-floss/frequenz-core-python#13
• frequenz-floss/frequenz-core-python#15
• frequenz-floss/frequenz-core-python#14

[llucax]

I reopened this because the suggested changes were not made. Even if the extra validation is not added, at least having tests for this would be good:

I would also add some tests for corner cases, like using plain ints, using an empty range, like Bounds(lower=0, upper=0)

If you think this should not be done, it would be good to give a reason.

[llucax]

griffe 0.46.1 released, re-running the failed job.

[llucax]

OK, the CI was fixed, and most comments were addressed. Since @jh2007github jumped to another topic and I want to move this to core, I will just merge and we can leave any improvements for future PRs (in core).

[llucax]

Queued cross-arch tests failed:

https://github.com/frequenz-floss/frequenz-sdk-python/actions/runs/9610219272/job/26506246519#step:10:891

I was debugging this for several hours last week and these LastValueCache tasks should not be pending, I checked and they are stopped before the test end. It is also super weird that this error consistently appears in the test_actor.py::test_restart_on_unhandled_exception[10] test, not in test_actor.py::test_restart_on_unhandled_exception[2] or [1], and the pending tasks are created by another by another tests (test_power_distributing.py IIRC), so it seems to be an isolation issue with pytest-async.

I guess it also depends on timing, otherwise we wouldn't have it fail only in one of the 2 matrix jobs of the cross-arch tests...

[llucax]

I wonder if it is related to this:

• Stop using a deprecated way to replace the event loop in tests #975

I think I will give it a try.

[llucax]

• Stop using a deprecated way to replace the event loop in tests #976

Will rebase when it is merged and see what happens, now I can't even reproduce locally (but before I could with 100% success rate).

[llucax]

@daniel-zullo-frequenz that introduces one extra merge. I will force-push with a rebase instead. @jh2007github FYI

[daniel-zullo-frequenz]

that introduces one extra merge. I will force-push with a rebase instead

Yes, I'm aware of it. I accidentally pressed the update-branch button. Sorry!

[llucax]

Sadly this is still failing in the same matrix job the same way. It is super weird, but the fact that only fails with this PR in a consistent way makes me very uncomfortable. I debugged this for hours and still could find how this might fail. We could have a workaround in the failing test to ignore logs from asyncio, but 😬

[llucax]

Giving this a new shot, in case it is some bug in some dependency and it was fixed, as this only happens with pytest_max.

[llucax]

[llucax]

I'm giving up for now and will just filter out the logs for the test to pass:

• Filter out irrelevant logs when checking for expected logs #983

But I created an issue about the whole thing, because I've seen other warnings seem to be flagging real issues:

• Look into test warnings #982

[llucax]

Will rebase again

[llucax]

Finally! This ended up being much more complicated to merge than expected...

[shsms]

It was just python demonstrating its newest async test capabilities.