Skip to content

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented May 12, 2025

The find_first_descendant_component accepts a root_category and finds
a component matching that category to begin traversing from. This PR
ensures that the matching root component that's found is as close as
possible to the GRID component.

Copilot AI review requested due to automatic review settings May 12, 2025 16:18
@shsms shsms requested a review from a team as a code owner May 12, 2025 16:18
@shsms shsms requested review from Marenz and removed request for a team May 12, 2025 16:18
@github-actions github-actions bot added part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid labels May 12, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the behavior of find_first_descendant_component so that when searching for a component with a non‑GRID root category, the method uses the grid component as the base and returns the closest matching descendant.

  • Introduces a separate branch for GRID components that directly selects the component from self.components.
  • For non‑GRID components, validates the existence of a grid component and then recursively locates the appropriate descendant.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/frequenz/sdk/microgrid/component_graph.py Adjusted find_first_descendant_component method for deterministic grid component look‑up.
RELEASE_NOTES.md Updated release notes to describe the fixed bug in the find_first_descendant_component method.
Comments suppressed due to low confidence (1)

src/frequenz/sdk/microgrid/component_graph.py:877

  • [nitpick] Consider reusing the already obtained grid_component to search for the descendant instead of making a recursive call, which would reduce redundant graph traversals.
root_component = self.find_first_descendant_component(root_category=ComponentCategory.GRID, descendant_categories=[root_category])

@shsms shsms enabled auto-merge May 12, 2025 16:23
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other comments are optional, but I also wonder what happens with islands, we are supposed to support islands. Isn't there any way to find descendants if there is no grid connection?

@shsms shsms disabled auto-merge May 13, 2025 08:12
@shsms
Copy link
Contributor Author

shsms commented May 13, 2025

I'm actually wondering why we need to support this root_category == Meter, etc. The root category is the GRID. The two places where this function is being used are specifying GRID as the root component.

Unless you're against, I'll remove this parameter. But if we keep it instead, we should reject everything that's not the GRID. The rest are not "root", because there can be multiple of them. But there can only be one GRID component in the graph.

@shsms
Copy link
Contributor Author

shsms commented May 13, 2025

Ideally this function is replaced by a bfs, but I will not do that now.

The `find_first_descendant_component` accepted a `root_category` where
categories could be specified that were not really the root of the
graph.

This feature was not necessary (and was never used) and was buggy.  It
is removed in this commit.

Signed-off-by: Sahas Subramanian <[email protected]>
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels May 13, 2025
@shsms shsms enabled auto-merge May 14, 2025 08:38
@shsms shsms requested a review from llucax May 14, 2025 08:38
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms added this pull request to the merge queue May 14, 2025
@github-project-automation github-project-automation bot moved this from To do to Review approved in Python SDK Roadmap May 14, 2025
@llucax
Copy link
Contributor

llucax commented May 14, 2025

Ideally this function is replaced by a bfs, but I will not do that now.

There was some bfs implemented by @matthias-wende-frequenz somewhere, IIRC.

Merged via the queue into frequenz-floss:v1.x.x with commit 1130c6a May 14, 2025
5 checks passed
@shsms shsms deleted the graph-fix branch May 14, 2025 09:02
@github-project-automation github-project-automation bot moved this from Review approved to Done in Python SDK Roadmap May 14, 2025
@shsms
Copy link
Contributor Author

shsms commented May 14, 2025

There was some bfs implemented by @matthias-wende-frequenz somewhere, IIRC.

No, he made a dfs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

2 participants