Conversation
github-actions
bot
added
part:tests
And add a temporary `test_loading` function to keep pytest from failing. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The `src` dir will be used for the rust bindings Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
I just gave a quick look to the Python interface. I know probably changes there also imply changes in the rust part, so they might not be trivial, so I'm also OK to go with it and leave changes for future PRs. I think the only thing I'm more concerned about is the component ID accepting anything that can be converted to int, as it could be error prone.
In terms of naming, now that this is about electrical components, I wonder if we should include it in the name, at least for consistency (I guess there will be no communication components graph, but who knows).
|
|
||
| Returns: | ||
| The PV coalesced formula as a string. | ||
| """ |
There was a problem hiding this comment.
I guess there is no way to convert a "regular" formula to a coalesce one to avoid the duplicates, right? Also any reasons to only provide coalesce formulas for grid, pv and batteries?
There was a problem hiding this comment.
I guess there is no way to convert a "regular" formula to a coalesce one to avoid the duplicates, right?
No, but maybe we can look into that later.
Also any reasons to only provide coalesce formulas for grid, pv and batteries?
Those were the only ones required for one of our internal apps for which these were added. I expect only the grid coalesce formula being useful for voltage, frequency streams in the SDK, but might not be too easy, need to look into that as well, later.
shsms
force-pushed
the
maturin
branch
8 times, most recently
from
af8d68c to
b239659
Compare
shsms
force-pushed
the
maturin
branch
2 times, most recently
from
851fa92 to
612ba6b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds Python bindings for the Frequenz microgrid component graph Rust library, enabling Python applications to use the graph functionality for managing microgrid electrical components.
Key changes:
- Migrated from setuptools to maturin build system for Rust/Python integration
- Added comprehensive Rust source files implementing Python bindings via PyO3
- Introduced multi-platform CI/CD pipeline supporting Windows, macOS, and Linux across multiple Python versions
Reviewed Changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml, Cargo.lock |
New Rust package configuration for the Python bindings |
src/lib.rs, src/graph.rs, src/component.rs, src/connection.rs, src/category.rs, src/utils.rs |
Rust implementation of Python bindings using PyO3 |
python/frequenz/microgrid_component_graph/__init__.py, __init__.pyi |
Python package exports and type stubs |
tests/test_microgrid_component_graph.py |
Updated tests for graph creation functionality |
pyproject.toml |
Build system changed from setuptools to maturin with new dependencies |
.github/workflows/ci.yaml |
Extended CI pipeline for multi-platform builds |
README.md |
Updated supported platforms documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
Besides these comments, I was thinking about package names in general. In particular on the Python side, in the long term I would like to have a frequenz.microgrid package that contains everything related to microgrids.
Now that we have more APIs, I'm not entirely clear if this should include the assets API, but I'm pretty clear it should probably not include the reporting API, so my feeling is it should not include the assets API either, so basically we have one frequenz.<api> package, so this one would be just about the microgrid API.
In this context, this should probably not have microgrid in the package name. Maybe it should be electrical component graph instead, to remove the microgrid and keep it more in sync with names in common?
Anyway, we don't need to resolve this in this PR, but before moving forward with a caotic structure again, I think it is worth having a quick chat to discuss how we want the repo/packages structure to look in the long term so we start new stuff with the right foot.
shsms
force-pushed
the
maturin
branch
2 times, most recently
from
38d1bae to
25c821d
Compare
shsms
force-pushed
the
maturin
branch
6 times, most recently
from
09b86c0 to
c0df9e5
Compare
| - name: Download distribution files | ||
| uses: actions/download-artifact@v6 | ||
| with: | ||
| name: dist-packages |
There was a problem hiding this comment.
Why removing the name? I guess it will download all artifacts if name is not present, including the generated docs website if it is there?
There was a problem hiding this comment.
Yes, only for the create release and publish stages, because there's a lot of artifacts and we can't use matrix, this was the easy alternative. Both stages will only use the wheel files.
There was a problem hiding this comment.
I see. You might be able to use globs, but not sure.
There was a problem hiding this comment.
This wasn't enough, need to build with maturin-action, to build with python's manylinux images: #18 (comment)
I merged it, because it got released as v0.1.0, instead of as rc during my test. Might need improvements, but we can look at it again later when we move this to repo-config
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
The remaining comments are super minor so approving, feel free to force-merge if you fix them.
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Also add tests to make sure graph creation and formula creation work. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
No description provided.