Fallback components in generated formulas #1037

ela-kotulska-frequenz · 2024-08-16T07:00:50Z

Fallback components are used in generated formulas. If primary components is unavailable, formula will generate metric from fallback components. Fallback formulas are implemented for:

PVPowerFormula
ProducerPowerFormula
BatteryPowerFormula
ConsumerPowerFormula
GridPowerFormula

All necessary formulas are implemented in this PR

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py

llucax

I just gave this a quick look. Will leave it for people more familiar with the formula engine.

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py

src/frequenz/sdk/timeseries/formula_engine/_formula_engine.py

src/frequenz/sdk/timeseries/formula_engine/_formula_steps.py

shsms

The logic looks good to me.

Would be nice if you put the docstrings and comments through a grammar checker like https://languagetool.org/ or https://www.grammarly.com/grammar-check.

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py

src/frequenz/sdk/timeseries/formula_engine/_formula_steps.py

llucax · 2024-08-21T12:33:43Z

...equenz/sdk/timeseries/formula_engine/_formula_generators/_fallback_formula_metric_fetcher.py

+    def start(self) -> None:
+        """Initialize formula engine and start fetching samples."""
+        self._formula_engine = self._formula_generator.generate()  # type: ignore[assignment]
+        assert isinstance(self._formula_engine, FormulaEngine)
+        self._receiver = self._formula_engine.new_receiver()


Mmm, interesting, but with this implementation stop() and cancel() will not work. I guess cancel() could still be a NOP, but you should probably override stop().

stop() should work because it has :

if not self._tasks: return

OK, I guess I reviewed this too quickly and didn't realized the missing stop() in the comment was this stop. I also missed that even when it had a run() method it didn't actually have any task running. I think I saw a run() and stop() and immediately assumed it had some task running.

I think this class is actually a good match for the new abstract Service interface, and a good case for splitting the ServiceBase with a basic implementation using tasks, as it doesn't really help in this case but it is nice to still have the common Service interface with start/stop/is_running/etc. for consistency.

Ok, I will do that later today (or on Monday).

frequenz-core-python has no release, yet.
Should I add dependency to the v1.x.x?

Maybe I could do that in separate PR (this one is pretty big now)?

No, sorry, I didn't mean to do it now, but I actually got thinking about this, and I wonder why isn't this just a Receiver with a couple of extra methods and we are already using stop() for some receivers (like in Merger.

Part of the reasons of splitting this to core is so we can use core in channels too, and maybe make these Receivers a Service too.

So my suggestion would be to make it a Receiver (so fetch_next() would just be receive()) and follow the interface of Service so in the future when this can inherit from Service it is not a breaking change.

OR, you can leave things are they are and we take care of it when we introduce the dependency to core, which will probably need adapting many other classes :)

This can't be Receiver, because stopping Receiver won't stop formula.
Here i would like to start/stop formula when needed. This means: subscribe/unsubscribe from all formula components, too.

I still don't get this.

So there is no stop() for now (or there is one inherited from BackgroundService that doesn't do anything).

I guess in the future you want to add a stop(). You are saying this stop() should not stop the self._formula_engine? Or the self._receiver? Or both? In any case, if you make FallbackMetricFetcher a Receiver, why can't you just make stop() do whatever it should do? If it is a semantic issue, maybe you could call it pause() instead of stop() to better convey that it is not entirely stopped but just not producing values anymore temporarily?

I think it was also mentioned before, I find it confusing that we have this extra inheritance FallbackFormulaMetricFetcher -> FallbackMetricFetcher. Can't we get rid of FallbackMetricFetcher? What is it for? We already have some unnecessary inheritance in the SDK from the past, I would prefer not to introduce more unless it is really necessary.

Done, I made it Receiver.

shsms

Still looking into it, but got a couple of items.

tests/timeseries/_battery_pool/test_battery_pool.py

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py

ela-kotulska-frequenz · 2024-08-26T12:09:07Z

first commit: _get_metric_fallback_components method is wrong, fixing it now.

...equenz/sdk/timeseries/formula_engine/_formula_generators/_fallback_formula_metric_fetcher.py

FallbackMericFetcher stores fallback formula but generates and subscribes for it on demand. Signed-off-by: Elzbieta Kotulska <[email protected]>

Change PVPowerFormula to get primary and fallback of the given components. For the fallback comonents sub formulas are created This makes sure that both main and fallback formulas have the same behaviour and less complexity. Signed-off-by: Elzbieta Kotulska <[email protected]>

Signed-off-by: Elzbieta Kotulska <[email protected]>

This receiver is only to forward messages to subscribers. It doesn't need more then 1 last message. No tests are broken after this. Signed-off-by: Elzbieta Kotulska <[email protected]>

Signed-off-by: Elzbieta Kotulska <[email protected]>

Previously if no_meter was True, meter was created but didn't send values. Now no meter is created. No tests are failing after this change Signed-off-by: Elzbieta Kotulska <[email protected]>

Signed-off-by: Elzbieta Kotulska <[email protected]>

And fix graph in test_batter_pool_power_two_batteries_per_inverter, Previously meters were connected to the grid and had no successors, inverters were connected to grid and had no meter. Now meters are connected to the inverters. Signed-off-by: Elzbieta Kotulska <[email protected]>

Fix mockMicrogrid: create senders always in the same order. Previous solution was working because we had simple graphs. Signed-off-by: Elzbieta Kotulska <[email protected]>

Signed-off-by: Elzbieta Kotulska <[email protected]>

This should simplify code Signed-off-by: Elzbieta Kotulska <[email protected]>

llucax · 2024-08-30T07:15:55Z

We need it because of circular imports :( We would have: FormulaGenerator -> FormulaEngine -> MetricFetcher -> FallbackFormulaMetricFetcher -> FormulaGenerator

Now we have FormulaGenerator -> FallbackFormulaMetricFetcher -> FallbackMetricFetcher FormulaGenerator -> FormulaEngine -> MetricFetcher -> FallbackMetricFetcher

#1037 (comment)

I added comment to the FallbackFormulaMetricFetcher: 23299b4#diff-9800776f7bac0eccbc7d0779b356f024334b154255b892b353dd85c69201f1bfR15

I see 😢

shsms · 2024-08-30T09:13:53Z

The switch to Receiver interface for FallbackMetricFetcher has made it much simpler and opens the door for more future cleanup.

I'd like to see two more changes, can be in this PR or can be turned into an issue:

I feel the latest_sample field in FallbackFormulaMetricFetcher doesn't belong there. It took me a while to figure out how it was used in the MetricFetcher.

Instead, I'd like to see a variable _latest_fallback_sample in the MetricFetcher itself, because that's where it is used. So keeping it entirely there would make it easier to follow when it gets set as well as when it gets read. And the FallbackMetricFetcher.latest_sample field can go away.
Then we can rename the FallbackMetricFetcher abc to LazyReceiver and move it to _internal._channels. And FallbackFormulaMetricFetcher can be renamed to FallbackFormulaMetricReceiver.

NB: if you're doing it in this PR, please put in a new commit, so it is easier to read the changes.

shsms

This is great, one more really cool SDK feature!

Approving from my side, but please wait for @llucax's approval also.

Because that's where it is used. Signed-off-by: Elzbieta Kotulska <[email protected]>

ela-kotulska-frequenz · 2024-08-30T10:58:15Z

@sahas-subramanian-frequenz

I feel the latest_sample field in FallbackFormulaMetricFetcher doesn't belong there. It took me a while to figure out how it was used in the MetricFetcher.

Done (last commit)

Then we can rename the FallbackMetricFetcher abc to LazyReceiver and move it to _internal._channels. And FallbackFormulaMetricFetcher can be renamed to FallbackFormulaMetricReceiver.

In separate PR: #1056

shsms · 2024-08-30T11:25:13Z

@sahas-subramanian-frequenz

I don't use that account anymore, so I won't be notified if you use it.

The latest commit looks good as well.

llucax

This is great, one more really cool SDK feature!

➕ 1

Thanks for taking care of it!

ela-kotulska-frequenz added part:tests Affects the unit, integration and performance (benchmarks) tests priority:high Address this as soon as possible type:enhancement New feature or enhancement visitble to users part:data-pipeline Affects the data pipeline labels Aug 16, 2024

ela-kotulska-frequenz self-assigned this Aug 16, 2024

ela-kotulska-frequenz requested a review from a team as a code owner August 16, 2024 07:00

ela-kotulska-frequenz requested review from Marenz and removed request for a team August 16, 2024 07:00

ela-kotulska-frequenz linked an issue Aug 16, 2024 that may be closed by this pull request

Fallback components in generated formulas in LogicalMeter #184

Closed

github-actions bot added the part:docs Affects the documentation label Aug 16, 2024

ela-kotulska-frequenz requested review from llucax, shsms, daniel-zullo-frequenz and matthias-wende-frequenz August 16, 2024 07:05

ela-kotulska-frequenz changed the title ~~Fallback components~~ Fallback components in formula Aug 16, 2024

ela-kotulska-frequenz changed the title ~~Fallback components in formula~~ Fallback metric in formula Aug 16, 2024

ela-kotulska-frequenz force-pushed the fallback_components branch 2 times, most recently from 39526b0 to 54cb9c8 Compare August 16, 2024 09:46

Marenz reviewed Aug 19, 2024

View reviewed changes

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py Outdated Show resolved Hide resolved

ela-kotulska-frequenz force-pushed the fallback_components branch from 54cb9c8 to d86115a Compare August 20, 2024 11:27

ela-kotulska-frequenz changed the title ~~Fallback metric in formula~~ Fallback components in generated formulas Aug 20, 2024

llucax reviewed Aug 20, 2024

View reviewed changes

shsms reviewed Aug 20, 2024

View reviewed changes

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py Outdated Show resolved Hide resolved

src/frequenz/sdk/timeseries/formula_engine/_formula_steps.py Outdated Show resolved Hide resolved

ela-kotulska-frequenz force-pushed the fallback_components branch from d86115a to 9af5416 Compare August 21, 2024 11:45

llucax reviewed Aug 21, 2024

View reviewed changes

ela-kotulska-frequenz force-pushed the fallback_components branch 3 times, most recently from b5bfa34 to db41689 Compare August 23, 2024 15:08

shsms reviewed Aug 26, 2024

View reviewed changes

tests/timeseries/_battery_pool/test_battery_pool.py Show resolved Hide resolved

src/frequenz/sdk/timeseries/formula_engine/_formula_generators/_formula_generator.py Outdated Show resolved Hide resolved

llucax reviewed Aug 30, 2024

View reviewed changes

...equenz/sdk/timeseries/formula_engine/_formula_generators/_fallback_formula_metric_fetcher.py Outdated Show resolved Hide resolved

...equenz/sdk/timeseries/formula_engine/_formula_generators/_fallback_formula_metric_fetcher.py Outdated Show resolved Hide resolved

ela-kotulska-frequenz added 15 commits August 30, 2024 09:15

Add FallbackMetricFetcher to MetricFetcher

a1ff91a

FallbackMericFetcher stores fallback formula but generates and subscribes for it on demand. Signed-off-by: Elzbieta Kotulska <[email protected]>

Fix tests after previous commit

91ad3e9

Signed-off-by: Elzbieta Kotulska <[email protected]>

Change receiver limit in mock_resampler

2a515d1

This receiver is only to forward messages to subscribers. It doesn't need more then 1 last message. No tests are broken after this. Signed-off-by: Elzbieta Kotulska <[email protected]>

Test pv_power formula with fallback components

94db914

Signed-off-by: Elzbieta Kotulska <[email protected]>

Update release notes

18c0572

Signed-off-by: Elzbieta Kotulska <[email protected]>

Fix MockMicrogrid add_chps method to can create chp without meter

7f24fa2

Previously if no_meter was True, meter was created but didn't send values. Now no meter is created. No tests are failing after this change Signed-off-by: Elzbieta Kotulska <[email protected]>

Implement fallback feature for ProducerPowerFormula

2ed5975

Signed-off-by: Elzbieta Kotulska <[email protected]>

Remove redundant parentheses in BatteryPowerFormula

778e5e4

Signed-off-by: Elzbieta Kotulska <[email protected]>

Add fallback formula feature for BatteryPowerFormula

8f567ba

Signed-off-by: Elzbieta Kotulska <[email protected]>

Add tests for BatteryPowerFormula fallback

226d628

Fix mockMicrogrid: create senders always in the same order. Previous solution was working because we had simple graphs. Signed-off-by: Elzbieta Kotulska <[email protected]>

Add fallback formula feature to ConsumerPowerFormula

c9d613e

Signed-off-by: Elzbieta Kotulska <[email protected]>

Add fallback formula feature to GridPowerFormula

aab30ee

Signed-off-by: Elzbieta Kotulska <[email protected]>

Use SimplePowerFormula in ProducerPowerFormula

1731ad4

This should simplify code Signed-off-by: Elzbieta Kotulska <[email protected]>

ela-kotulska-frequenz force-pushed the fallback_components branch from ff0c626 to 1731ad4 Compare August 30, 2024 07:16

shsms previously approved these changes Aug 30, 2024

View reviewed changes

ela-kotulska-frequenz mentioned this pull request Aug 30, 2024

Improvement of fallback metric fetcher #1056

Open

Move latest_sample from FallbackFormulaMetricFetcher to MetricFetcher

c70ea97

Because that's where it is used. Signed-off-by: Elzbieta Kotulska <[email protected]>

ela-kotulska-frequenz dismissed shsms’s stale review via c70ea97 August 30, 2024 10:57

llucax approved these changes Aug 30, 2024

View reviewed changes

ela-kotulska-frequenz added this pull request to the merge queue Aug 30, 2024

Merged via the queue into frequenz-floss:v1.x.x with commit 82dcb63 Aug 30, 2024
18 checks passed

ela-kotulska-frequenz deleted the fallback_components branch August 30, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback components in generated formulas #1037

Fallback components in generated formulas #1037

ela-kotulska-frequenz commented Aug 16, 2024 •

edited

Loading

llucax left a comment

shsms left a comment

llucax Aug 21, 2024

ela-kotulska-frequenz Aug 21, 2024

llucax Aug 22, 2024

ela-kotulska-frequenz Aug 23, 2024 •

edited

Loading

ela-kotulska-frequenz Aug 23, 2024 •

edited

Loading

llucax Aug 26, 2024

ela-kotulska-frequenz Aug 26, 2024

llucax Aug 28, 2024

ela-kotulska-frequenz Aug 29, 2024

shsms left a comment

ela-kotulska-frequenz commented Aug 26, 2024

llucax commented Aug 30, 2024

shsms commented Aug 30, 2024

shsms left a comment

ela-kotulska-frequenz commented Aug 30, 2024

shsms commented Aug 30, 2024

llucax left a comment

Fallback components in generated formulas #1037

Fallback components in generated formulas #1037

Conversation

ela-kotulska-frequenz commented Aug 16, 2024 • edited Loading

llucax left a comment

Choose a reason for hiding this comment

shsms left a comment

Choose a reason for hiding this comment

llucax Aug 21, 2024

Choose a reason for hiding this comment

ela-kotulska-frequenz Aug 21, 2024

Choose a reason for hiding this comment

llucax Aug 22, 2024

Choose a reason for hiding this comment

ela-kotulska-frequenz Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

ela-kotulska-frequenz Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

llucax Aug 26, 2024

Choose a reason for hiding this comment

ela-kotulska-frequenz Aug 26, 2024

Choose a reason for hiding this comment

llucax Aug 28, 2024

Choose a reason for hiding this comment

ela-kotulska-frequenz Aug 29, 2024

Choose a reason for hiding this comment

shsms left a comment

Choose a reason for hiding this comment

ela-kotulska-frequenz commented Aug 26, 2024

llucax commented Aug 30, 2024

shsms commented Aug 30, 2024

shsms left a comment

Choose a reason for hiding this comment

ela-kotulska-frequenz commented Aug 30, 2024

shsms commented Aug 30, 2024

llucax left a comment

Choose a reason for hiding this comment

ela-kotulska-frequenz commented Aug 16, 2024 •

edited

Loading

ela-kotulska-frequenz Aug 23, 2024 •

edited

Loading

ela-kotulska-frequenz Aug 23, 2024 •

edited

Loading