Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Do not merge] POC: Use SDK component graph and formula engine with reporting client #102

Draft
wants to merge 9 commits into
base: v0.x.x
Choose a base branch
from

Conversation

cwasicki
Copy link
Contributor

@shsms and I implemented this POC to show how the SDK's component graph and formula engine can be used with the reporting API:

  • Only for demo purpose, not intended to be merged.
  • Also not intended to go into this repo, but probably into https://github.com/frequenz-floss/frequenz-reporting-python.
  • Still needs a lot of clean-up.
  • Moreover it's a temporary solution until this functionality will be provided by the SDK in the future.

@cwasicki cwasicki requested review from llucax and shsms September 23, 2024 19:49
@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Sep 23, 2024
@llucax
Copy link

llucax commented Sep 24, 2024

A few high-level notes:

  • I was planning to move the component graph to the microgrid API client. I think it makes sense, it would be very weird that when dealing with the microgrid you don't want to deal with a component graph instead of individual connections. This also means we need much less wrapping, as we would not need to wrap the Connection message for example. But I don't know now if we want to move the "component graph" to a rust library. I have the feeling we are talking about a higher-level there, right? It is more like the formula engine and how to use the component graph to come up with some formulas, no?
  • The commit Adapt component graph that copies a lot of wrapper classes should be not necessary if those wrappers live in frequenz-api-common (or some other similar package), so that's another thing that we could remove from here (and that's not in the SDK).
  • About the Copy quantities and base types from SDK v1.0.0-rc901 commit
    • We already have https://github.com/frequenz-floss/frequenz-quantities-python/, I just noticed it is at rc already, I guess I only had pending to integrate it to the SDK before releasing 1.0 final, no changes should be expected, so I think it should be safe to start using it here too.
    • Not sure what to do with Sample and friends, if we need a separate repo for that or if we can put it also in frequenz-api-common or however it is called.
    • About Bounds, I already moved them to https://github.com/frequenz-floss/frequenz-core-python/ as Interval to have it more generic, I think we can use it in many places, for example the components in the new microgrid API (and common API, so for sure reporting too) has a Lifetime (operational_lifetime) object that is also basically a Interval[datetime]. This repo still needs some work before a 1.0 and have no releases yet, but we could make a development v0.1.0 release or something if you want to use it here.
    • SystemBounds not sure if we should move it to core too, I think I have a draft implementation locally but never made a PR because I was implementing it as a list of intervals, but apparently system bounds will be limited to only 2 intervals to also represent a inclusion and exclusion zone. This probably needs a bit more thought.
  • Commit Add sdk bridge: I didn't understand very well what that is.
  • About copying formula engine, I guess this we want to implement in rust long term, right? Maybe Sample belongs there? Not sure.
  • Wasn't the idea to put this PR in https://github.com/frequenz-floss/frequenz-reporting-python instead?

@cwasicki
Copy link
Contributor Author

I was planning to move the component graph to the microgrid API client.

Does that mean that in the cloud we would need the microgrid API client if we want to use the component graph?

But I don't know now if we want to move the "component graph" to a rust library.

Another motivation to have it in rust is that it is needed by the grid guard.

The commit Adapt component graph that copies a lot of wrapper classes should be not necessary if those wrappers live in frequenz-api-common

That would be good, but will take some time I guess.

We already have https://github.com/frequenz-floss/frequenz-quantities-python/,

About Bounds, I already moved them to https://github.com/frequenz-floss/frequenz-core-python/

Cool, didn't know about these! Maybe we advertise these (and any other new repos) in our actors call.

Not sure what to do with Sample and friends, if we need a separate repo for that or if we can put it also in frequenz-api-common or however it is called.

Probably you considered that already, but couldn't that go with the quantities and we call it base types or so?

Commit Add sdk bridge: I didn't understand very well what that is.

This translates the iterator of batches from the API into a receiver of samples to be used by the SDK tools.

About copying formula engine, I guess this we want to implement in rust long term, right?

Yes, this is needed in rust since it will be used by the reporting service. Same with the resampler.

Wasn't the idea to put this PR in https://github.com/frequenz-floss/frequenz-reporting-python instead?

Yes, this PR is just a raw upload of our hack for sharing purpose. The proper PR would go into the other repo.

@llucax
Copy link

llucax commented Oct 3, 2024

Does that mean that in the cloud we would need the microgrid API client if we want to use the component graph?

No "I was" 😆, I guess I won't anymore if the idea is to implement in rust and have it in a different repo.

But I don't know now if we want to move the "component graph" to a rust library.

Another motivation to have it in rust is that it is needed by the grid guard.

Yeah, when I wrote this I wasn't sure about the scope of that move, but now it is clear for me that the component graph will go to this rust repo, and it will take as arguments for building it some set of components and connections, which can come from the reporting or microgrid API (or a CVS file or whatever).

The commit Adapt component graph that copies a lot of wrapper classes should be not necessary if those wrappers live in frequenz-api-common
That would be good, but will take some time I guess.

Yes, I plan to do it after I finish with the microgrid client v0.17, then we can try to merge what's already there, what reporting has and what I wrote for the microgrid API.

About Bounds, I already moved them to frequenz-floss/frequenz-core-python
Cool, didn't know about these! Maybe we advertise these (and any other new repos) in our actors call.

Yeah, this is just one of my many truncated projects, I was going to announce it when it was ready, but I didn't though I was going to abandon it for this long. Maybe we can start using it now, but there is no release yet for core. quantities we can already start using I think, I released a RC, my plan was to integrating it to the SDK to validate it worked as expected, but again, I never did, but it would be good to validate in other projects too I guess.

Probably you considered that already, but couldn't that go with the quantities and we call it base types or so?

I'm not crazy about that, quantities is a very generic library as it is, that has nothing to do with samples. For example it can be used for trading stuff. I think the idea is also now to re-write the resampler as a rust library, maybe Sample can live there?

@cwasicki
Copy link
Contributor Author

cwasicki commented Oct 7, 2024

I'm not crazy about that, quantities is a very generic library as it is, that has nothing to do with samples. For example it can be used for trading stuff. I think the idea is also now to re-write the resampler as a rust library, maybe Sample can live there?

IMO a sample is also something very generic, that is not only related to the resampler but useful anywhere where we have a data point with a timestamp (observations/measurements, forecasts), i.e. in all our apps. However, I am considering adding a time series data structure which might reduce some of the pain with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants