feat(assets): Add Support for Electrical Component Connections

[eduardiazf]

Summary

Add a new client method to retrieve microgrid electrical components connections. For this a new ComponentConnection class is introduced. The CLI is also extended to add a command to list component-connections, for example: assets-cli component-connections 123 --source 5 --source 6 --destination 10

Resources

• https://github.com/frequenz-floss/frequenz-client-microgrid-python/blob/v0.18.x/src/frequenz/client/microgrid/component/_connection.py

[llucax]

LGTM, but I would improve the API as proposed. With this change not only it can accept lists of IDs, it can also accept tuples, sets, or any iterable, like dict.values() or dict.keys().

[cwasicki]

IIUC this would also collect Nones, is this intended?

[llucax]

True, good catch. I have this in the microgrid API:

        return (
            conn
            for conn in map(
                component_connection_from_proto,
                response.electrical_component_connections,
            )
            if conn is not None
        )

An a more functional alternative 😄

        return list(
            map(
                component_connection_from_proto,
                filter(bool, response.electrical_component_connections),
            )
        )

[eduardiazf]

Thanks, good catch.

[cwasicki]

Is this an issue or a valid case?

[llucax]

In the microgrid API is valid, and for now it is always returned without a lifetime, but maybe this needs to change in the future, not sure.

We should probably document in the protobuf files what to expect though. Some of these issues I added them only to be notified if they happened or not because I couldn't find a strict definition.

Once we have a clear definition, we might want to remove them (for example, if it is a defined thing that a missing operational lifetime can happen and then the meaning is it is always operational, we don't need to be whining about it when we see it happen, it will be just noise.

@tiyash-basu-frequenz FYI

[tiyash-basu-frequenz]

This is already documented in the latest API version: https://github.com/frequenz-floss/frequenz-api-common/blob/300bcb4296d679faa3b2aec33e7ea3d461d90beb/proto/frequenz/api/common/v1alpha8/microgrid/lifetime.proto#L21-L30

Therefore, the phrase "missing operational lifetime, considering it always operational" is technically correct. I do not think it should be classified as an issue though.

[ktickner]

is there a reason it is this concise? It's nice to reduce cognitive load of messages, but this is verging on being easily misunderstood.

[llucax]

Therefore, the phrase "missing operational lifetime, considering it always operational" is technically correct. I do not think it should be classified as an issue though.

Yeah, it is is expected behavior it is not an issue and we should remove it, as it will just spam the logs. I will remove it from the microgrid API client too.

is there a reason it is this concise? It's nice to reduce cognitive load of messages, but this is verging on being easily misunderstood.

What are you talking about exactly, what's concise?

[cwasicki]

Do we want to consider it as operational? If so, I don't see a way how we can detect these cases programatically. Maybe we add a way to check how many major issues were found to understand how reliable the connections are.

[llucax]

Do we want to consider it as operational?

Good question, this was done with the microgrid API in mind, where right now everything is operational all the time basically, but maybe it is not safe to assume it operational.

If so, I don't see a way how we can detect these cases programatically. Maybe we add a way to check how many major issues were found to understand how reliable the connections are.

The question for me is if there is anything we can do (and want to do) programatically. I think in general an error here is a misconfiguration, and the best we can do is tell the user (in this case an error should be logged) so they can fix it.

Then we probably need to try to recover as best as possible, so there is the question if we ignore the connection (consider it non-operational) or we add it (assume it is operational). Both cases will be wrong some times and right some other times, so I guess there is no good answer for this, but maybe ignoring it is more correct, as if one field is wrong, maybe other fields are wrong too.

Or back to your question, what else do you think it can be done by a client if the wrong connection would be passed to the user of the client? Do you envision clients applying some heuristics to try to do a better guess on how to treat a faulty connection?

[tiyash-basu-frequenz]

See here.

[cwasicki]

I agree that it makes sense to define missing as always operational. What I am not sure about is whether an invalid lifetime should be accepted and treated as missing. Invalid indicates a user error to me.

[cwasicki]

Or back to your question, what else do you think it can be done by a client if the wrong connection would be passed to the user of the client? Do you envision clients applying some heuristics to try to do a better guess on how to treat a faulty connection?

I guess my main point would be to have a way to validate whether the graph could be parsed without any issues or not, because atm we only have a warning message. It depends on the use-case if you prefer to have reliable data or better some data. Also you could try to recover it or have fallbacks in place if a problem was detected.

[llucax]

I think the graph will be constructed from the UI, and there will be no way to construct it in an invalid way, so I guess this code is mostly about really weird situations, memory corruption or something like this, so I think as long as we log an error (I think major issues are logged as warnings now, we might want to reconsider that, or have a separate critical_issues that are logged as errors), it doesn't matter much how we deal with it.

[llucax]

That said, probably if part of a message is broken it would make sense to ignore the whole message instead try to guess fix it.

[llucax]

I think you removed the wrong "issue", this issue should still be checked and reported (I think it would probably be better to just skip the whole connection object, so return None, if the operational lifetime is invalid. If it is missing, then that's OK, so that issue should not be reported.

[cwasicki]

If there is no supported way to construct invalid graphs and it's only weird cases, are these then valid at all? If not, why not just raise an exception in this case?

[llucax]

I didn't understand the last comment. Maybe we could meet to quickly discuss this.

[Marenz]

Hmm a bit meta but it I found it a bit cumbersome to read every time "This commit introduced/updates/.." it feels very redundant. Nothing against AI generated if that is the case, but maybe ask it to cut it down a bit? :)

[llucax]

Hmm a bit meta but it I found it a bit cumbersome to read every time "This commit introduced/updates/.." it feels very redundant. Nothing against AI generated if that is the case, but maybe ask it to cut it down a bit? :)

I send this, I also find the PR description too verbose, to the point that my brain unconsciously tries to skip it. For me something more succinct would be more useful, like:

Add a new client method to retrieve microgrid electrical components connections. For this a new ComponentConnection class is introduced. The CLI is also extended to add a command to list component-connections, for example: assets-cli component-connections 123 --source 5 --source 6 --destination 10

I think the rest of the details can be read from the PR itself.

Maybe you can tweak the instructions to generate PRs/commit messages, which makes me think that maybe it is time to start providing custom instructions, as I guess we can all benefit from having good instructions to produce useful PR/commit messages.

[llucax]

I think you removed the wrong "issue", this issue should still be checked and reported (I think it would probably be better to just skip the whole connection object, so return None, if the operational lifetime is invalid. If it is missing, then that's OK, so that issue should not be reported.

[llucax]

This should not accept None, otherwise we need to do special casing here, is better to keep it as it was before, that a Lifetime() is just always operational. Also in user code, if users need to access operational lifetime directly they will need to deal with None as a possibility now.

Suggested change

	operational_lifetime: Lifetime | None = None
	"""The operational lifetime of the connection."""
	operational_lifetime: Lifetime
	"""The operational lifetime of the connection."""

[llucax]

Suggested change

	if self.operational_lifetime is None:
	return True

[llucax]

Suggested change

	return None
	return Lifetime()

[cwasicki]

What is actually the motivation for collecting those messages instead of logging them immediately? I wonder why we need this additional complexity.

[llucax]

Flexibility. Doing this actually makes supporting your use case pretty trivial. The client could just return the issues instead of logging them. This code is supposed to go to client-common.

[cwasicki]

If this is the idea, then these issues are probably better tracked via enums instead of strings. If you prefer to not do it in this PR then it should come in the next one. Because in the current form we cannot use this endpoint in our trading app.

[eduardiazf]

We can add it in the next one because this implementation should be moved to client-common.

[cwasicki]

Sounds good!