[Draft] Implement FileClient2, which raises on error

[hannesweisbach]

This PR aims to resolve issue #327. Currently it is in Draft status.

Open questions/issues:

• List request does not seem to have an error code? Is this a bug in the Cyphal Spec?
• Use OSError or custom Exception type? (possibly derived from OSError)
• docstrings need to be updated
• possibly more tests for exceptions (mocked?)
• server-side CI
• general feedback regarding coding style/guidelines (when not already mentioned by CI)
• should FileClient2.get_info() also raise on error?

There is a test which tries to write /bin/sh under Linux, which I don't like. I don't think that's a good idea, because it just might succeed. After all, nox prompts for a sudo password ...

[coveralls]

coverage: 94.744% (+0.07%) from 94.679%
when pulling e9c9bac on hannesweisbach:issue-327
into 765ab8d on OpenCyphal:master.

[pavel-kirienko]

Just a quick review of the draft. Please mark the old FileClient deprecated.

[pavel-kirienko]

I'm not sure about the use of OSError as the base as it is designed for a different class of errors. The most obvious issue is that the first argument is errno while we feed it our own error code, which does not have to match the error code assignment of errno. Then it also invites for the use of string aliases; here we already have a problem in that we don't handle the case when error.value is not in _perror_uavcan, which is a defect in itself.

I suggest we remove the perror mapping and switch from OSError to something more generic, perhaps just Exception.

[hannesweisbach]

Yeah, I'm not sure either. OSError uses the errno to return subclasses of itself, according to PEP 3151. I'm kind of leaning towards translating Cyphal error codes to PEP 3151 exception types directly, without use of OSError. I think the following mapping is natural:

Error.NOT_FOUND: -> FileNotFoundError
Error.ACCESS_DENIED: -> PermissionError
Error.IS_DIRECTORY: -> IsADirectoryError

Unfortunately that leaves the rest of the error codes still without solution. I think a pro-argument for using PEP 3151 exception types would be that these exceptions are already defined and intended for this use case. The con-argument could be that the exception type is not clearly marked as belonging to (py)cyphal. This might make them catching by type tricky (for example: "catch all Exceptions that FileClient2.read() might raise").

There is precedence for using exceptions from PEP 3151 in pycyphal. NetworkTimeoutError inherits from TimeoutError, which was introduced as part of PEP 3151, afaict.

Then it also invites for the use of string aliases;

What do you mean?

here we already have a problem in that we don't handle the case when error.value is not in _perror_uavcan, which is a defect in itself.

I think it should be covered by a unit test that all pre-defined values in uavcan.file.Error should have a human-readable error string. Or would you prefer not having a textual error description at all? How would you propose translating the numeric error code into a human readable string?

I suggest we remove the perror mapping and switch from OSError to something more generic, perhaps just Exception

I was thinking about a class FileError or something similar:

class FileError(Exception):
  def __init__(self, error: Error):
    super().__init__(<where to get this message?>)
    self.error_code = error.value # or something similar

I think a case can be made for the exception carrying the numeric error value from Cyphal: unit tests. Not having the numeric value in the exception also makes writing unit tests harder: either just assert that an exception was raised, assert on the content of the error string, or only the fact that an exception was raised. The latter two seem somewhat fragile.

with pytest.raises(FileError) as e:
  ... # code under test
assert e.error_code == Error.FILE_TOO_LARGE

[pavel-kirienko]

Ok, how about this: building upon the precedence of having NetworkTimeoutError inherit from TimeoutError which inherits from OSError, we define a separate exception type per error code. For those error codes that have a corresponding standard exception type, we inherit directly from said exception type as you suggested. For the others we inherit from OSError, initializing it with the appropriate standard errno in the constructor. Behold an example:

# Make NetworkTimeoutError inherit this too!
class RemoteFileError:
    """
    This is a tag type used to differentiate Cyphal remote file errors.
    """
    # Optionally, we can make it a dataclass and add additional metadata:
    remote_error_code: int
    remote_node_id: int

# There is a predefined exception type for this error; we use that.
class RemoteFileNotFoundError(FileNotFoundError, RemoteFileError):
    def __init__(self, name: str) -> None:
        super().__init__(errno.ENOENT, "NOT_FOUND", name)

# There is no predefined exception type for this error; use OSError.
class RemoteOutOfSpaceError(OSError, RemoteFileError):
    def __init__(self, name: str) -> None:
        super().__init__(errno.ENOSPC, "OUT_OF_SPACE", name)

In such a fashion, we define an exception type per error code. Next, we define a factory that maps Cyphal error codes to error instances:

_ERROR_MAP: dict[int, Callable[[str], OSError]] = {
    Error.OUT_OF_SPACE: RemoteOutOfSpaceError,
    Error.NOT_FOUND: RemoteFileNotFoundError,
    # ...
}
"""Maps error -> path -> exception"""

def _map_remote_error_code_to_exception(remote_error_code: int, path: str) -> OSError:
    try:
        return _ERROR_MAP[remote_error_code](path)
    except LookupError:  # Last resort handler -- unknown error code
        return OSError(errno.EPROTO, f"Unknown remote error code: {remote_error_code}", path)

I think it should be covered by a unit test that all pre-defined values in uavcan.file.Error should have a human-readable error string.

The remote end will then be able to trigger an internal error in your application by returning a custom error code.

[hannesweisbach]

Hello Pavel and thank you for your quick replies.

I think this is a good solution. I'll implement it shortly.

[hannesweisbach]

    # Optionally, we can make it a dataclass and add additional metadata:
    remote_error_code: int
    remote_node_id: int

I think it is a good idea, but I'm not sure how to pass the arguments correctly to/from super().__init__(). Could it be neither @dataclass not OSError pass additional arguments in their __init__()? Even super(RemoteFileError, self).__init__() gave me errors; but I'm not a Python developer.

Regardless, I think adding attributes to a class is a non-breaking changing and can thus be implemented later if there is a need.

Regarding the LookupError:

def _map_remote_error_code_to_exception(remote_error_code: int, path: str) -> OSError:
    try:
        return _ERROR_MAP[remote_error_code](path)
    except LookupError:  # Last resort handler -- unknown error code
        return OSError(errno.EPROTO, f"Unknown remote error code: {remote_error_code}", path)

I'm not sure about hiding that error, because that behavior is hard to document. Also it is not really an OSError. I would propose:

     try:
         return _ERROR_MAP[error.value](path)
     except KeyError as e:
         raise Exception(
             f'Unknown error code reported from remote {error}. Are you running different Cyphal versions? '
             'If not, please report this issue to pycyphal.'
         ) from e

I think notifying the developer/user that something is amiss is the right thing to do. I think this should fail very rarely, if ever.
I also added a unit test, which tries to check, that for each attribute

[pavel-kirienko]

I'm not sure about hiding that error, because that behavior is hard to document.

The error is not hidden but reported via OSError; it is ultimately caused by the remote end (the file server) reporting an error code we don't know of. We could choose a different exception type but there is value in consistency; one wishing to catch all exceptions caused by a remote end will be able to do so by catching OSError.

While we're at it, the class documentation should explain that all exceptions caused by the remote end inherit both from OSError and RemoteFileError.

I think this should fail very rarely, if ever.
I also added a unit test, which tries to check, that for each attribute

This is irrelevant. This error can be triggered by the file server returning a custom/unknown error code at no fault of the local client. The reference to the Cyphal version in the error message is also incorrect because a new error code may appear within the same major Cyphal version (actually it has no relation to the protocol version).

[hannesweisbach]

The error is not hidden but reported via OSError;

I think I meant "shadowed" rather then "hidden". But I wasn't aware that is ok to send custom error codes.

While we're at it, the class documentation should explain that all exceptions caused by the remote end inherit both from OSError and RemoteFileError.

Done.

The reference to the Cyphal version in the error message is also incorrect because a new error code may appear within the same major Cyphal version (actually it has no relation to the protocol version).

I wasn't aware, but good to know.

[pavel-kirienko]

Please use ReST formatting.

Suggested change

	In contrast to pycyphal.application.file.FileClient,
	pycyphal.application.file.FileClient2 raises exceptions for errors reported
	over the network. The intent is to expose better error handling in the API;
	especially avoid ``isintance()`` tests on return values.
	In contrast to :class:`FileClient`,
	:class:`FileClient2` raises exceptions for errors reported
	over the network.
	The intent is to expose more pythonic error handling in the API.

[hannesweisbach]

I'm not familiar with it, but I'll give it a try :)

[hannesweisbach]

Just a quick review of the draft. Please mark the old FileClient deprecated.

Done.

[pavel-kirienko]

Looks good. Please check my tiny nitpick and bump the minor version number. For bonus points please drop a line in the changelog. Thanks!

[pavel-kirienko]

Suggested change

	if error.value == Error.OK:
	return
	raise _map(error, filename)
	if error.value != Error.OK:
	raise _map(error, filename)

[hannesweisbach]

Done. Thanks for the review!