refactor(asyncio): Fix misc python issues

1) Use the proper logging namespace "remotivelabs.broker", so that
loggers initialized with logging.getLogger(__name__) works as
expected.

2) Load version from pyproject.toml file, so that version string
is not duplicated in __about__.py.

3) Add imports of generated files to the root package, so that
they are properly encapsulated in the library. Also make sure
our higher-level abstractions in sync/ package use that import.

4) Clean up tests and add some new.

python/README.md

@@ -48,9 +48,7 @@ cd python/remotivelabs-broker
 
 ## Versioning
 
-Update the package version by editing the following files:
-- `pyproject.toml`
-- `remotivelabs/broker/__about__.py`
+Update the package version by editing the following `pyproject.toml` file.
 
 Follow [Semantic versioning](https://semver.org/). Beta versions should be suffixed with `b*`, example `0.2.0b1`.
 

python/remotivelabs-broker/docker/build.sh

@@ -32,14 +32,14 @@ function generate_protobuf_files() {
   mkdir -p "${PROTO_STUBS_OUT}"
 
   poetry run python -m grpc_tools.protoc \
-    -I $PROTO_IN \
+    --proto_path=$PROTO_IN \
     --python_out=$PROTO_STUBS_OUT \
     --grpc_python_out=$PROTO_STUBS_OUT \
     --mypy_out=$PROTO_STUBS_OUT \
     --mypy_grpc_out=$PROTO_STUBS_OUT \
     $PROTO_IN/*.proto
 
-  # TODO: fix this
+  # Note: protobuf compiler does not support generating relative or custom absolute imports for python. Use a script to do this manually...
   poetry run python $PROJECT_DIR/misc/fix_import_statements.py
 }
 

python/remotivelabs-broker/misc/fix_import_statements.py

@@ -7,6 +7,11 @@
 #
 # This script goes through all the python files in the folder and does the replacement based on the regex pattern
 # `regex_string` defined below.
+#
+# See also:
+# - https://github.com/grpc/grpc/issues/9575
+# - https://github.com/protocolbuffers/protobuf/issues/1491
+# - https://github.com/protocolbuffers/protobuf/issues/5374
 
 import glob
 import re

python/remotivelabs-broker/pyproject.toml

@@ -1,7 +1,7 @@
 [tool.poetry]
 name = "remotivelabs-broker"
 description = 'RemotiveLabs Broker gRPC API'
-version = "0.1.26"
+version = "0.2.0"
 readme = "README.md"
 license = "Apache-2.0"
 homepage = "https://remotivelabs.com/"
@@ -53,7 +53,6 @@ pdoc = "^12.2"
 json-schema-for-humans = "^1.0"
 
 [tool.poe.tasks]
-test-local = "pytest -m 'not server' --cov=remotivelabs.broker"
 test-server = "pytest -m server --cov=remotivelabs.broker"
 test = "pytest --cov=remotivelabs.broker"
 format = [{ cmd = "ruff check --select I --fix" }, { cmd = "ruff format" }]
@@ -62,7 +61,7 @@ format-check = [
   { cmd = "ruff format --check --diff" },
 ]
 mypy-check = [{ cmd = "mypy remotivelabs/broker" }, { cmd = "mypy tests" }]
-check = ["test-local", "format-check", "mypy-check"]
+check = ["test", "format-check", "mypy-check"]
 
 [tool.ruff]
 line-length = 140
@@ -76,7 +75,10 @@ namespace_packages = true
 include = ["remotivelabs"]
 
 [tool.pytest.ini_options]
-markers = ["server: Run test towards a live server."]
+addopts = "-v -m 'not server'"
+markers = [
+    "server: marks tests that require a live server (deselect with '-m \"not server\"')",
+]
 
 [build-system]
 requires = ["poetry-core>=1.6"]

python/remotivelabs-broker/remotivelabs/broker/__about__.py

@@ -1,5 +1,9 @@
 # SPDX-FileCopyrightText: 2022-present remotiveLabs <[email protected]>
 #
 # SPDX-License-Identifier: Apache-2.0
+from importlib.metadata import PackageNotFoundError, version
 
-__version__ = "0.1.26"
+try:
+    __version__ = version("remotivelabs-broker")
+except PackageNotFoundError:
+    __version__ = "unknown"

python/remotivelabs-broker/remotivelabs/broker/__init__.py

@@ -1,37 +1,51 @@
 """
-remotiveLabs Python API for remotiveBroker.
-See `version` below.
+RemotiveLabs Python API for RemotiveBroker.
+
+This API uses protobuffer and gRPC stubs directly, available in the submodules:
+- `remotivelabs.broker.common_pb2`.
+- `remotivelabs.broker.common_pb2_grpc`.
+- `remotivelabs.broker.diagnostics_api_pb2`.
+- `remotivelabs.broker.diagnostics_api_pb2_grpc`.
+- `remotivelabs.broker.functional_api_pb2`.
+- `remotivelabs.broker.functional_api_pb2_grpc`.
+- `remotivelabs.broker.network_api_pb2`.
+- `remotivelabs.broker.network_api_pb2_grpc`.
+- `remotivelabs.broker.system_api_pb2`.
+- `remotivelabs.broker.system_api_pb2_grpc`.
+- `remotivelabs.broker.traffic_api_pb2`.
+- `remotivelabs.broker.traffic_api_pb2_grpc`.
 
 In addition to return codes, this package uses logging to convey operational
-status. Logging is done to the name space "com.remotivelabs.broker".
+status. Logging is done to the namespace "remotivelabs.broker".
 
-As a user, enable basic logging in your application with:
 
 ```python
-logging.basicConfig()
+# Disable logging for this package:
+logging.getLogger("remotivelabs.broker").propagate = False
 ```
-
-Disable logging for this package:
-
-```python
-logging.getLogger("com.remotivelabs.broker").propagate = False
-```
-
-Use sub module: `remotivelabs.broker.sync`.
 """
 
 # SPDX-FileCopyrightText: 2022-present remotiveLabs <[email protected]>
 #
 # SPDX-License-Identifier: Apache-2.0
-
-import logging
-
 from .__about__ import __version__
-
-log: logging.Logger = logging.getLogger("com.remotivelabs.broker")
-"""Package logging interface"""
-
-log.addHandler(logging.NullHandler())
+from ._log import configure_logging
+from .generated.sync import (
+    common_pb2,  # noqa: F401
+    common_pb2_grpc,  # noqa: F401
+    diagnostics_api_pb2,  # noqa: F401
+    diagnostics_api_pb2_grpc,  # noqa: F401
+    functional_api_pb2,  # noqa: F401
+    functional_api_pb2_grpc,  # noqa: F401
+    network_api_pb2,  # noqa: F401
+    network_api_pb2_grpc,  # noqa: F401
+    system_api_pb2,  # noqa: F401
+    system_api_pb2_grpc,  # noqa: F401
+    traffic_api_pb2,  # noqa: F401
+    traffic_api_pb2_grpc,  # noqa: F401
+)
 
 version: str = __version__
 """Library version"""
+
+configure_logging()

python/remotivelabs-broker/remotivelabs/broker/_log.py

@@ -0,0 +1,11 @@
+"""
+Configure logging according to best practicies for libraries
+
+https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library
+"""
+
+import logging
+
+
+def configure_logging() -> None:
+    logging.getLogger("remotivelabs.broker").addHandler(logging.NullHandler())

python/remotivelabs-broker/remotivelabs/broker/sync/__init__.py

@@ -3,26 +3,13 @@
 
 Create a connection with the method `remotivelabs.broker.sync.create_channel`.
 
-This API uses protobuffer and gRPC stubs directly. Which are availble in the submodules:
-- `remotivelabs.broker.sync.common_pb2`.
-- `remotivelabs.broker.sync.common_pb2_grpc`.
-- `remotivelabs.broker.sync.diagnostics_api_pb2`.
-- `remotivelabs.broker.sync.diagnostics_api_pb2_grpc`.
-- `remotivelabs.broker.sync.functional_api_pb2`.
-- `remotivelabs.broker.sync.functional_api_pb2_grpc`.
-- `remotivelabs.broker.sync.network_api_pb2`.
-- `remotivelabs.broker.sync.network_api_pb2_grpc`.
-- `remotivelabs.broker.sync.system_api_pb2`.
-- `remotivelabs.broker.sync.system_api_pb2_grpc`.
-- `remotivelabs.broker.sync.traffic_api_pb2`.
-- `remotivelabs.broker.sync.traffic_api_pb2_grpc`.
-
 For an example on how to use these we recommend looking at the samples for this library.
 Which is available at the repository remotiveLabs samples:
 
 Link: <https://github.com/remotivelabs/remotivelabs-samples/tree/main/python>.
 """
 
+# Prefer import from root module. We keep this import for backwards compatibility.
 from ..generated.sync import (
     common_pb2,  # noqa: F401
     common_pb2_grpc,  # noqa: F401

python/remotivelabs-broker/remotivelabs/broker/sync/client.py

@@ -8,8 +8,8 @@
 
 import grpc
 
-from ..generated.sync import network_api_pb2 as network_api
-from ..generated.sync import (
+from .. import network_api_pb2 as network_api
+from .. import (
     network_api_pb2_grpc,
     system_api_pb2_grpc,
     traffic_api_pb2_grpc,
@@ -116,7 +116,6 @@ def __iter__(self):
     def __next__(self):
         try:
             result = self.signals[self.index]
-        # pylint: disable=raise-missing-from
         except IndexError:
             raise StopIteration
         self.index += 1
@@ -199,7 +198,7 @@ def to_protobuf_signal(s: SignalIdentifier):
                 lambda sub: (wait_for_subscription_queue.put((self.client_id, sub))),
             ),
         ).start()
-        # Wait for subscription
+
         client_id, subscription = wait_for_subscription_queue.get()
         return subscription
 
@@ -213,7 +212,6 @@ def _on_signals(self, signals_in_frame: network_api.Signals, callback):
             self.on_signals(SignalsInFrame(list(map(SignalValue, signals_in_frame))))  # type: ignore[call-overload]
 
     def list_signal_names(self) -> List[SignalIdentifier]:
-        # Lists available signals
         configuration = self._system_stub.GetConfiguration(br.common_pb2.Empty())
 
         signal_names: List[SignalIdentifier] = []

python/remotivelabs-broker/remotivelabs/broker/sync/helper.py

@@ -2,6 +2,7 @@
 
 import hashlib
 import itertools
+import logging
 import ntpath
 import os
 import posixpath
@@ -12,15 +13,16 @@
 import grpc
 from grpc_interceptor import ClientCallDetails, ClientInterceptor
 
-from .. import log
-from ..generated.sync import (
+from .. import (
     common_pb2,
     network_api_pb2,
     network_api_pb2_grpc,
     system_api_pb2,
     system_api_pb2_grpc,
 )
 
+_logger = logging.getLogger(__name__)
+
 
 class HeaderInterceptor(ClientInterceptor):
     def __init__(self, header_dict):
@@ -101,9 +103,8 @@ def publish_signals(client_id, stub, signals_with_payload, frequency: int = 0) -
 
     try:
         stub.PublishSignals(publisher_info)
-    # pylint: disable=protected-access
-    except grpc._channel._Rendezvous as err:  # type:ignore[attr-defined]
-        log.error(err)
+    except grpc._channel._Rendezvous:  # type:ignore[attr-defined]
+        _logger.exception("A Rendezvous error occurred")
 
 
 def printer(signals: Sequence[common_pb2.SignalId]) -> None:
@@ -114,7 +115,7 @@ def printer(signals: Sequence[common_pb2.SignalId]) -> None:
     """
 
     for signal in signals:
-        log.info(f"{signal} {signal.namespace.name}")
+        _logger.info(f"{signal} {signal.namespace.name}")
 
 
 def get_sha256(path: str) -> str:
@@ -153,13 +154,13 @@ def upload_file(system_stub: system_api_pb2_grpc.SystemServiceStub, path: str, d
     """
 
     sha256 = get_sha256(path)
-    log.debug(f"SHA256 for file {path}: {sha256}")
+    _logger.debug(f"SHA256 for file {path}: {sha256}")
     with open(path, "rb") as file:
         # make sure path is unix style (necessary for windows, and does no harm om
         # linux)
         upload_iterator = generate_data(file, dest_path.replace(ntpath.sep, posixpath.sep), 1000000, sha256)
         response = system_stub.UploadFile(upload_iterator, compression=grpc.Compression.Gzip)
-        log.debug(f"Uploaded {path} with response {response}")
+        _logger.debug(f"Uploaded {path} with response {response}")
 
 
 def download_file(system_stub: system_api_pb2_grpc.SystemServiceStub, path: str, dest_path: str) -> None:
@@ -204,7 +205,7 @@ def reload_configuration(
 
     request = common_pb2.Empty()
     response = system_stub.ReloadConfiguration(request, timeout=60000)
-    log.debug(f"Reload configuration with response {response}")
+    _logger.debug(f"Reload configuration with response {response}")
 
 
 def check_license(
@@ -237,8 +238,7 @@ def act_on_signal(
     :param fun: Callback for receiving signals update
     :param on_subscribed: Callback for successful subscription
     """
-
-    log.debug("Subscription started")
+    _logger.debug("Subscription started")
 
     sub_info = network_api_pb2.SubscriberConfig(
         clientId=client_id,
@@ -249,25 +249,24 @@ def act_on_signal(
         subscripton = network_stub.SubscribeToSignals(sub_info, timeout=None)
         if on_subscribed:
             on_subscribed(subscripton)
-        log.debug("Waiting for signal...")
+
+        _logger.debug("Waiting for signal...")
         for subs_counter in subscripton:
             fun(subs_counter.signal)
 
     except grpc.RpcError as e:
         # Only try to cancel if cancel was not already attempted
-        # pylint: disable=no-member
         if e.code() != grpc.StatusCode.CANCELLED:
             try:
                 subscripton.cancel()
-                print("A gRPC error occurred:")
-                print(e)
+                _logger.exception("A gRPC error occurred")
             except grpc.RpcError:
                 pass
-    # pylint: disable=protected-access, bad-except-order
-    except grpc._channel._Rendezvous as err:  # type:ignore[attr-defined]
-        log.error(err)
+    except grpc._channel._Rendezvous:  # type:ignore[attr-defined]
+        _logger.exception("A Rendezvous error occurred")
+
     # reload, alternatively non-existing signal
-    log.debug("Subscription terminated")
+    _logger.debug("Subscription terminated")
 
 
 def act_on_scripted_signal(
@@ -289,7 +288,7 @@ def act_on_scripted_signal(
     :param on_subscribed: Callback for successful subscription
     """
 
-    log.debug("Subscription with mapping code started...")
+    _logger.debug("Subscription with mapping code started...")
 
     sub_info = network_api_pb2.SubscriberWithScriptConfig(
         clientId=client_id,
@@ -300,20 +299,20 @@ def act_on_scripted_signal(
         subscription = network_stub.SubscribeToSignalWithScript(sub_info, timeout=None)
         if on_subscribed:
             on_subscribed(subscription)
-        log.debug("Waiting for signal...")
+
+        _logger.debug("Waiting for signal...")
         for subs_counter in subscription:
             fun(subs_counter.signal)
 
-    except grpc.RpcError as e:
+    except grpc.RpcError:
+        _logger.exception("A gRPC error occurred")
         try:
             subscription.cancel()
-            print("A gRPC error occurred:")
-            print(e)
         except grpc.RpcError:
             pass
 
-    # pylint: disable=protected-access, bad-except-order
-    except grpc._channel._Rendezvous as err:  # type:ignore[attr-defined]
-        log.error(err)
+    except grpc._channel._Rendezvous:  # type:ignore[attr-defined]
+        _logger.exception("A Rendezvous error occurred")
+
     # reload, alternatively non-existing signal
-    log.debug("Subscription terminated")
+    _logger.debug("Subscription terminated")