Move consumer and producer formulas out from logical meter

[daniel-zullo-frequenz]

Move consumer and producer power formulas from logical meter to their own logical components Consumer and Producer, respectively.

Related to #782

[llucax]

LGTM. I know the documentation discussion is probably out of scope of this PR, I'd be fine also merging as is and addressing the docs in another PR, although maybe it is worth moving the explanation of what Consumer and Producer is from the method to the class, that shouldn't be too much work.

[llucax]

I don't find this docs very useful. I was trying to come up with a description¹ but the concept of consumer is non trivial, so I thought we should probably add a cross-reference to the Glossary, but then I realized the glossary is also outdated or incomplete, because we have only a definition of Consumption ( and Gross Consumption) that refers to the clipped value, and here we actually want to say that this groups everything that is not producing power (in a sense). I notice we are defining a Load in the glossary, and to be honest, IMHO this is the most common name for what we call consumer, is the part of the circuit we don't know about or control.

There is also the wiki glossary that we can take as reference: https://github.com/frequenz-floss/frequenz-sdk-python/wiki/Glossary#microgrid-power-terminology, for example it says:

The term consumer was deliberately chosen to indicate that this asset type is expected to predominantly consume electric power. Under normal circumstances there should be no active sources in this asset type and the metrics consumer_{consumption,production}_power would not be used. Alternative terms: load, demand, sink.

I guess if we don't want to get into the whole load/consumer discussion again, we could just add a Consumer entry in the glossary and that's it.

@frequenz-floss/python-sdk-team @cwasicki FYI

Footnotes

1. I also wouldn't mention the types returned, like FormulaReceiver, I would explain at a higher level what you can get from this (which for now is just power). ↩

[llucax]

OK, I see in here it is more clear, maybe we need to harmonize the docs of the class and property. I think it makes more sense to document in the class what the consumer is.

[llucax]

Same comments in general as for Consumer.

[llucax]

I did the review mostly by looking at the review comments I made in the past, I didn't do another in-depth review. With this in mind, LGTM!

[llucax]

I was wondering if it was worth adding a dependency for this, as it seems very trivial to implement, but then I looked at it and it is not that simple, the library considers inheritance for example, so I think it is worth having it as a dependency.

[daniel-zullo-frequenz]

Alternatively we could use @cached_property

It seems the only difference is a constrain that will be removed in python 3.12:

Changed in version 3.12: Prior to Python 3.12, cached_property included an undocumented lock
to ensure that in multi-threaded usage the getter function was guaranteed to run only once
per instance. However, the lock was per-property, not per-instance, which could result in
unacceptably high lock contention. In Python 3.12+ this locking is removed.

[daniel-zullo-frequenz]

I added a commit to replace @lazy with @cached_property as it seems to be what we needs and it is provided in the standard library. I can just remove the new commit if anyone is against it.

[llucax]

I knew about @cached_property but I thought it was deprecated, I must be confusing it with something else. In any case, @cached_property seems like a very different monster, as it allows writes:

The mechanics of cached_property() are somewhat different from property(). A regular property blocks attribute writes unless a setter is defined. In contrast, a cached_property allows writes.

The cached_property decorator only runs on lookups and only when an attribute of the same name doesn’t exist. When it does run, the cached_property writes to the attribute with the same name. Subsequent attribute reads and writes take precedence over the cached_property method and it works like a normal attribute.

I guess this is why all the extra complexity is needed, like locking, etc. It also says the __dict__ will take more space, and all sort of weird implications (which I guess for this case is not that bad).

But just the allowing to write will be problematic for us, as it allows users to overwrite (even if accidentally) the property. Oh, damn, I just did a test, and lazy allows writes too :(

class X:
    @cached_property
    def x(self) -> int:
        return 1

x = X()
print(x.x)
x.x = 10
print(x.x)

class Y:
    @lazy
    def y(self) -> int:
        return 1

y = Y()
print(y.y)
y.y = 10
print(y.y)

Both work and pass mypy without any errors. Not sure what to do now... Maybe we should delay this change and leave it as it was for now. I would also be fine with @cached_property if you prefer. @shsms any opinions from your side?

[daniel-zullo-frequenz]

We could write a customize ReadOnlyCachedProperty as follows:

class ReadOnlyCachedProperty(cached_property):
    def __set__(self, *args, **kwargs):
        raise AttributeError("Cannot assign a value to a read-only property")

class X:
    @ReadOnlyCachedProperty
    def x(self) -> int:
        return 1

x = X()
print(x.x)
x.x = 10 # AttributeError: Cannot assign a value to a read-only property

[shsms]

I don't like the idea of making them writable. They're outside the hot path normally, but I can see the value in caching anyway, because someone might just write really bad code, and that might be very expensive, and unintuitive, because they're just using a property.

The (probably not scalable) approach that I've been following has been to cache in a private global variable and use that.

but just to add one more option into the mix:

class X:
    @property
    def x(self):
        @cache
        def _x():
            # call constructor
        return _x()

or add a from_cache_or_create constructor in underlying class

class SlowConstructable:
    @cache
    @classmethod
    def from_cache_or_create(cls, ...args):
        return cls(...args)

class X:
    @property
    def x(self)
        SlowConstructable.from_cache_or_create(args)

[llucax]

About Dani's approach, I like that is simple, but I don't like that we'll still use cached_property which is a very complicated beast for something that should be much simpler.

About Sahas' approach I'm not crazy about moving the caching logic to the "upstream" class, if someone wants to cache anything, it should be a downstream problem.

If we really want to go with this, I would either implement the decorator from scratch or something like a LazyValue class so we can do:

class X:
    def __init__(self, ...) -> None:
        self._x = LazyValue(lambda: TheValueClass(...))
    @property
    def x(self):
        return self._x.value

where LazyValue is something like:

class LazyValue[T]:
    def __init__(self, constructor: Callable[[None], T], /) -> None:
        self._value: T | None = None
        self._ctor: Callable[[None], T] = constructor
    @property
    def value(self) -> T:
        if self._value is None:
            self._value = self._ctor()
        return self._value

(we can get away with None here because it would be quite stupid to construct a None lazily... :D)

The decorator approach is definitely the best IMHO in terms of readability and usability though...

But IMHO this went a bit out of hand, it was just a minor suggestion for this PR but totally unrelated. I would just remove the last 3 commits from the PR and merge it, and then decide what to do about this in a separate PR.

Before this PR they were already cached, we just had a lot of duplicate code just checking if the object was already constructed or not and construct it if it weren't (IIRC).

[daniel-zullo-frequenz]

About Dani's approach, I like that is simple, but I don't like that we'll still use cached_property which is a very complicated beast for something that should be much simpler.

I think we could also do it for @lazy

Before this PR they were already cached, we just had a lot of duplicate code just checking if the object was already constructed or not and construct it if it weren't (IIRC).

That's right.

But IMHO this went a bit out of hand, it was just a minor suggestion for this PR but totally unrelated. I would just remove the last 3 commits from the PR and merge it, and then decide what to do about this in a separate PR.

Agree. I think we just need to assess the implications of each solution as the implementation itself should be relatively simple. I'll remove the last 3 commits and create an issue

[daniel-zullo-frequenz]

Created the issue https://github.com/frequenz-floss/frequenz-sdk-python/issues/856

[daniel-zullo-frequenz]

Removed the last 3 commits but kept the slight changes to docstring in _data_pipelines for the "lazy" existent getters in a new commit