Add specific gRPC client errors

[llucax]

This makes error handling more pythonic, as one can now just catch the exception type one is interested in, without having to do a second-level matching using the status.

It also helps avoiding to expose the grpclib classes to the user.

[llucax]

This is marked as a draft because it is based on #49 and it needs tests, but it is pretty much ready for a review, and I would appreciate early feedback, as I will probably move this to client-base once it is proved to work. @frequenz-floss/python-sdk-team

[llucax]

Enabling auto-merge.

[shsms]

Optional, but I usually like to organize stuff with comments, when there are so many things inside __all__, for example:

__all__ = [
    # 
    # Generic types
    # 
    "Component",
    "ComponentCategory",
    # 
    # Streaming types
    # 
    "BatteryData",
    # 
    # Exceptions
    # 
    "DataLoss",
]

[llucax]

Doesn't that info comes from the imports already? For me this is all very verbose and duplicated, I really would like to move to use from m import * in __init__.py and that's it. It works as expected with mypy (and official in Python, is part of the typing PEP, PEP484), but sadly mkdocstrings doesn't support it yet, so we still need to use __all__ if we want symbols to appear in the docs 😢

[shsms]

The list of symbols in __all__ is much more readable from the jumbled import statements where the symbols are not aligned and are grouped by the file they come from and not the category. Please don't remove it.

[llucax]

OK, this is a separate conversation, so let's not have it in this PR, I prefer to focus on this PR here, so I will just focus on having comments in __all__ or not.

For me __all__ is just noise, another way to do pylint: disable=unused-import or whatever is called. I think the key is to match files to categories, this is basically why we split symbols in different files, so for me looking at the import already gives me all the information I need. IMHO adding the comments to __all__ is adding even more duplication, now I not only have to repeat the y of from x import y, I also have to repeat the x (and if we are not repeating it, we need to re-organize which symbol goes to which file IMHO, not fixing it with comments in __all__).

That said, if it really adds clarity for you, I can add it, I guess we should try to make stuff as readable as possible for as much people as possible 🤷 🙂

[shsms]

this is a separate conversation, so let's not have it in this PR, I prefer to focus on this PR here

I didn't start it 😈

For me all is just noise

and you didn't really stop either. 😠

if it really adds clarity for you, I can add it

I like doing it, but I mentioned optional anyway, so it is up to you.

[llucax]

One more thing while I think about this (I was looking into it to see if I would use a different categorization or the same as in files). If these comments are really intended to the users, I think we should add that classification in the module documentation instead, if we do that, we have some duplication, we still need to more or less list all the symbols twice, but I do see much more added value there, if this classification is shown in the docs.

In any case, this this is pre-existing code and not something added by this PR, I would rather do this as a separate PR, when improving the docs.

[llucax]

and you didn't really stop either. 😠

Haha, I meant just about removing __all__ completely.

if it really adds clarity for you, I can add it

I know see the value in the grouping as user documentation, but I don't want to get into writing the docs now that the project will change a lot when the upgrade to v0.17.0 happens.

If you think the current mapping of files/categories makes sense, I could add comments to match the files, that's not a problem as it is very low effort. Just let me know and I will add it, is just a few tab-tab with copilot.

If not, I will need to spend some time thinking which categories are appropriate, and if I do that, I will want to also map those categories to files as it should be. This will be something much more time consuming and again kind of pointless just before the upgrade to v0.17.0.

[shsms]

Maybe this can be UnrecognizedGrpcError?

[llucax]

Maybe. Do you think it would be useful to catch a UnrecognizedGrpcError? Otherwise I'm not sure what would be the advantage of giving it a special class. I can add it anyway, I'm not against it, mostly curious if you already had a use case in mind.

[shsms]

oh, didn't realize GrpcStatusError is a superclass for all the other errors. Why is it not just called GrpcError, btw?

[shsms]

I can add it anyway, I'm not against it, mostly curious if you already had a use case in mind.

No, it makes sense the way you've done it, thought the GrpcStatusError was a separate thing just for the unrecognized case. But also found the Status part of the error's name weird.

[llucax]

Yeah, actually because GrpcStatusError is a superclass for all other errors it makes sense to have this separately, otherwise there is no way to tell if the status we received is unrecognized, if you catch GrpcStatusError, then you'll catch all of the others.

I wondered if it shouldn't be just mapped to UnknownError, but I think it shouldn't because so far every know gRPC error status is mapped to a single class, and if we do so UnknownError could have a UNKNOWN status or any other random int that we don't know about as status, which is more confusing, so I guess a dedicated error for an unrecognized status still makes sense.

[llucax]

Added a new exception UnrecognizedGrpcStatus and added the subclass at the end of the Raises: docs in the client, I will squash once it is pre-approved.

[shsms]

and added the subclass at the end

I think it should be a subclass of ....

did you also see my question above about "Why not call it GrpcError?" The error is not with the status, but with grpc.

[llucax]

I think it should be a subclass of ....

OK, I can do that.

did you also see my question above about "Why not call it GrpcError?" The error is not with the status, but with grpc.

Why isn't the error with the status? The error is we don't recognize the status, right? I don't see how this is not an unrecognized status, then we could call it UnrecognizedGprcStatus or UnrecognizedGprcStatusError, but I'm trying to be consistent with other specific errors not having the Error suffix unless the class name makes sense only with it.

[shsms]

No, I meant for the superclass GrpcStatusError, GrpcError fits better. There is context above, I guess you missed it. UnrecognizedGprcStatus sounds fine.

[llucax]

No, I meant for the superclass GrpcStatusError, GrpcError fits better. There is context above, I guess you missed it. UnrecognizedGprcStatus sounds fine.

OK, I just wanted to emphasize that this error is basically mapping status codes to exceptions, I'm not sure if we could eventually want to raise other types of gRPC errors that are not tied to a status, like if you can't connect to the gRPC service at all, but maybe that is not strictly a gRPC error. I'm open to rename it to GrpcError if it makes more sense to you.

[llucax]

Updated with a subclass of... and a fix for the commit adding UnrecognizedGrpcStatus.

[shsms]

like if you can't connect to the gRPC service at all, but maybe that is not strictly a gRPC error

For that we have ClientError right? But likely we'll receive a 'TimedOut' or 'Unavailable' error from the grpc library.

I'm open to rename it to GrpcError if it makes more sense to you.

I think we should, to make it clear that a grpc error happened, and not something that happened while dealing with a status. When a service method fails without providing a status or has a panic or something, we'll see the status 'Unknown'. So there will always be a status. grpclib seems to call it grpclib.GRPCError too.

[llucax]

Pushed the rename. This should be ready for a final LGTM and then I can squash the fixups.

[shsms]

These need to change too :D

[llucax]

Ah, right. Now I hope all references are updated.

[llucax]

Now, updated

[llucax]

I just squashed and pushed because I guess you already did the review of the fixup commits.

[llucax]

Sorry, one last update to improve the release notes to adjust to the current changes.

[shsms]

This is awesome! We can take this further by defining more domain specific error statuses - either using the grpc status mechanism, don't know if that's possible, or through the description. For example,

except FailedPrecondition as e:
    if e.desc() == "OVER_SOC":
        # allow discharge, but block charge

but that would need to be coordinated with the server side.

[llucax]

Yeah, we can define more meaningful errors in the error description I guess. Or maybe the "details", never looked what is that for and if it can be easily set by the server. I would do it more structurally, and have more exceptions in the hierarchy so we don't need to resort again to inspecting the exception to have a look at another level of type of error, so more like:

except BatteryAlreadyFull:
    ...