Add up_rust_py package with documentation and examples#1
Add up_rust_py package with documentation and examples#1sachinkum0009 wants to merge 25 commits into
Conversation
sophokles73
left a comment
There was a problem hiding this comment.
I have provided a few hints and proposals for improvement. My Python knowledge is very limited so I the review of the python code might not be as thorough as you would expect.
I do like the clear structuring and that you already thought about including proper documentation 👍
In general, the Eclipse license header is missing in all files. You may want to take a look a the up-rust repository for the format of the header and where to add it.
| requires-python = ">=3.8" | ||
| description = "Python bindings for Eclipse uProtocol Rust implementation" | ||
| readme = "README.md" | ||
| license = { text = "Apache-2.0" } |
There was a problem hiding this comment.
From what I see at https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license-and-license-files this should probably be
license = "Apache-2.0"or am I mistaken?
There was a problem hiding this comment.
Thanks, you're right. I will fix it.
| mqtt = [] # MQTT transport (future) | ||
| someip = [] # SOME/IP automotive protocol (future) |
There was a problem hiding this comment.
IMHO we should omit these until there actually is support for them ...
| discovery = [] # Service discovery (udiscovery feature) | ||
| subscription = [] # Advanced subscription management (usubscription feature) | ||
| twin = [] # Digital twin support (utwin feature) | ||
| cloudevents = [] # CloudEvents format support |
There was a problem hiding this comment.
same here, FMPOV it is totally ok to only start with what is really supported. Once you add more bindings, e.g. for the uSubscription service interface, you can add a corresponding feature extension ..
|
|
||
| 1. Clone the repository: | ||
| ```bash | ||
| git clone https://github.com/sachinkum0009/up-rust-py.git |
There was a problem hiding this comment.
I guess this should become https://github.com/eclipse-uprotocol/up-rust-py.git, right?
| - Python 3.8 or higher | ||
| - pip (Python package installer) | ||
|
|
||
| ## Install from PyPI |
There was a problem hiding this comment.
IMHO we should add a section recommending to use a virtual environment?
|
|
||
| Here's a simple example that demonstrates publishing and receiving messages: | ||
|
|
||
| ```python |
There was a problem hiding this comment.
instead of pasting the code in here (where we will have a hard time keeping it in sync with the code base), we should probably simply refer to a file in the examples folder?
| entity ID, and version to create unique identifiers. | ||
| """ | ||
|
|
||
| def __init__(self, authority: str, entity_id: int, version: int) -> None: |
There was a problem hiding this comment.
what happens if I provide a version value that is out of range?
There was a problem hiding this comment.
I found out that the PyO3 automatically converts Python types to Rust Types with validation.
If the value is out of range, it will return a OverflowError.
| Will be called when messages arrive. | ||
|
|
||
| Raises: | ||
| Exception: If registration fails. |
There was a problem hiding this comment.
I am not too familiar with error handling in Python but Exception looks very generic to me. Isn't there a concept for having more specific errors being reported?
There was a problem hiding this comment.
Thanks, I created a custom Expection class to handle and map the error from the rust e.g, UStatusError.
To Do
|
…rity and organization
| - 🐍 **Pythonic API**: Easy-to-use interfaces that feel natural in Python | ||
| - ⚡ **Async Ready**: Built on Tokio async runtime for efficient I/O operations | ||
|
|
||
| ## Installation (Comming Soon) |
There was a problem hiding this comment.
If the package is not (yet) available from PyPi, then FMPOV we should not describe this as the (primary) way of installing the library, but add it once the package has been published for the first time. Otherwise, you will get questions why installation does not work as described ...
There was a problem hiding this comment.
Agree. I will update the Readme instructions.
| 1. **Build from source using virtualenv**: | ||
|
|
||
| ```bash | ||
| # activate virtualenv |
There was a problem hiding this comment.
I am not sure what this comment is supposed do mean. Do I need to manually activate virtualenv before, or does pip install maturing activate virtualenv?
There was a problem hiding this comment.
Yes, when using virtualenv. You have to manually activate virtualenv and then run the pip install command.
| - **PyO3 Bindings**: Zero-cost abstractions between Python and Rust | ||
| - **Python API**: Pythonic interfaces with full type hints | ||
|
|
||
| Each async operation maintains its own Tokio runtime for thread-safe execution across the Python/Rust boundary. |
There was a problem hiding this comment.
This sounds very expensive. Is this the recommended/established way to do this?
There was a problem hiding this comment.
You're right. I will search for different way.
|
|
||
| # Wait a bit more for messages to be received | ||
| print("\n3. Waiting for message delivery...") | ||
| time.sleep(2) |
There was a problem hiding this comment.
IMHO we should not sleep here but instead use some channel to signal that the receiver has actually received the message that had been sent.
| /// >>> text = message.extract_string() | ||
| /// >>> if text: | ||
| /// ... print(f"Received: {text}") | ||
| fn extract_string(&self) -> PyResult<Option<String>> { |
There was a problem hiding this comment.
this function seems to completely contradict the error handling logic from up_rust. Is this by intention? Unless there is a compelling reason to do so, I would propose to simply stick with the API provided by UMessage, e.g. extracting the payload as a byte array or deserializing ProtoBuf messages ...
Introduce the up_rust_py package, providing Python bindings for uProtocol. Include necessary modules, documentation, and example scripts for usage. This addition enhances the project's functionality and usability.