Add async support and optimize Python codegen#14
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors generation and serialization from a registry/descriptor model to class-based Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProtocolBase
participant Framer
participant Stream
Client->>ProtocolBase: send(message, async_=False)
ProtocolBase->>Framer: _encode_message(message) -> framed_bytes
ProtocolBase->>Stream: write(framed_bytes)
Note right of Stream: sync write completes
Client->>ProtocolBase: send(message, async_=True)
ProtocolBase->>Framer: _encode_message(message) -> framed_bytes
ProtocolBase->>Stream: async write coroutine -> await drain()
Note right of Stream: async writer drained
loop receive loop
ProtocolBase->>Stream: read (sync or await)
Stream-->>ProtocolBase: bytes
ProtocolBase->>Framer: append bytes
Framer-->>ProtocolBase: frame ready?
alt frame complete
ProtocolBase->>ProtocolBase: _decode_frame() -> Struct instance
ProtocolBase-->>Client: return/yield Struct
else not complete
ProtocolBase-->>Client: continue/backoff
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
emmapowers
force-pushed
the
modernize/2-python-modernization
branch
from
526979e to
51ddf34
Compare
emmapowers
force-pushed
the
modernize/3-python-codegen
branch
from
c724fc0 to
7def8d3
Compare
emmapowers
force-pushed
the
modernize/2-python-modernization
branch
from
51ddf34 to
fee9b06
Compare
emmapowers
force-pushed
the
modernize/3-python-codegen
branch
from
7def8d3 to
0759207
Compare
emmapowers
force-pushed
the
modernize/2-python-modernization
branch
from
fee9b06 to
34a256e
Compare
emmapowers
force-pushed
the
modernize/3-python-codegen
branch
from
0759207 to
afb3f51
Compare
- Add 'bakelite runtime -l python' command to generate runtime files - Support --runtime-import flag for custom import paths - Rewrite Python template for black/ruff/mypy compliance - Use BLANK_LINE variable for clearer template formatting - Generated code uses temporary sys.path modification for imports - Update arduino example with new runtime generation
- Add async_= parameter to send(), poll(), and messages() methods - Use @overload decorators for proper type hints - Support both BufferedIOBase (sync) and StreamReader/StreamWriter (async) - Add messages() as the primary iterator interface for consuming streams
- Replace decorator-based API with dataclass base classes (Struct, BakeliteEnum) - Generate inline pack/unpack code instead of runtime interpretation - Batch consecutive primitive fields into single struct.pack/unpack calls - Simplify ProtocolBase to use class attributes for message mapping
emmapowers
force-pushed
the
modernize/3-python-codegen
branch
from
afb3f51 to
7f9f728
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
bakelite/generator/cli.py (1)
20-27: Consider clarifying the--runtime-importhelp text.The current help text
"Import path for runtime. No value=bakelite.proto, omit=runtime"is cryptic. A clearer description would help users understand the three modes:
- Omit the flag entirely → uses
"bakelite_runtime"(relative import)--runtime-import(no value) → uses"bakelite.proto"(installed package)--runtime-import=my.module→ uses"my.module"(custom path)Suggested improvement
`@click.option`( "--runtime-import", "runtime_import", is_flag=False, flag_value="bakelite.proto", default=None, - help="Import path for runtime. No value=bakelite.proto, omit=runtime", + help="Runtime import path. Omit for relative bakelite_runtime, use without value for bakelite.proto, or specify custom path", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bakelite/generator/cli.py` around lines 20 - 27, Update the help string for the click option defined on runtime_import (the click.option call with "--runtime-import") to clearly describe the three behaviors: omitting the flag selects the relative import "bakelite_runtime", passing the flag with no value selects the installed-package import "bakelite.proto", and passing a value (e.g., --runtime-import=my.module) uses that custom import path; modify the help text to succinctly list these three modes and their resulting import values so users can understand the flag's three behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bakelite/generator/cli.py`:
- Around line 55-58: The cpptiny branch currently writes cpptiny.runtime() to
output_path assuming it's a file, which fails if output_path is the default ".";
update the language == "cpptiny" branch to detect if output_path is a directory
(use os.path.isdir(output_path)) and, if so, set a sensible filename (e.g.,
os.path.join(output_path, "bakelite.h")) before opening; otherwise use
output_path as given, and validate that the resolved path is not a directory
before calling open(...); modify the code around cpptiny.runtime(), output_path,
and the open(...) call accordingly.
In `@bakelite/generator/templates/python.py.j2`:
- Around line 7-9: The template is importing Enum explicitly in the conditional
blocks even though generated enum classes inherit from BakeliteEnum (which
already subclasses Enum); remove the redundant "from enum import Enum" import
lines in both conditional sections of the python.py.j2 template, ensure no
remaining references to the bare Enum name in the template (leave BakeliteEnum
as the base for generated enums), and run a quick generation check to confirm
enums still import/resolve via BakeliteEnum.
In `@bakelite/proto/runtime.py`:
- Around line 215-227: Both `_messages_sync` and `_messages_async` spin when
`poll()` returns None causing high CPU; update these functions so that when `msg
is None` they perform a short backoff instead of immediately looping: in
`_messages_sync`, call a blocking wait such as `time.sleep()` (or implement an
exponential backoff) after a missed poll; in `_messages_async`, call `await
asyncio.sleep()` similarly; alternatively, if `poll()` supports a blocking mode,
switch to using that (e.g., `self.poll(block=True)` / `await
self.poll(async_=True, block=True)`); ensure you import `time` and `asyncio` as
needed and keep yielding messages unchanged when `msg` is not None.
- Around line 157-166: The synchronous poll method reads from self._stream with
read() which blocks until EOF; change it to read a bounded size (e.g. 4096) like
the async path to avoid indefinite blocking on non-EOF streams (serial ports).
Update the call in poll (the code that currently calls self._stream.read()) to
pass a buffer size (4096) and keep the subsequent logic that appends to
self._framer and decodes via _framer.decode_frame() and _decode_frame(frame)
unchanged so behavior remains consistent with the async implementation.
- Around line 173-179: The _poll_async loop currently swallows all exceptions
from await self._async_reader.read(4096) which can hide real errors; update
_poll_async to stop using a bare except Exception: pass — either catch only
expected exceptions such as asyncio.IncompleteReadError (and handle
reconnect/EOF), or log unexpected exceptions via the module logger (including
exception info) and re-raise if it indicates a fatal condition; ensure any
caught error still allows safe cleanup and that successful reads continue to
call self._framer.append_buffer(data).
In `@bakelite/proto/serialization.py`:
- Around line 47-50: The wrapper around dataclasses.field currently prefers
default over default_factory and silently drops default_factory; change it to
detect when both default and default_factory are provided and raise the same
error dataclasses.field would (e.g., raise ValueError("cannot specify both
default and default_factory")), so update the wrapper function that returns
field(... ) to check if default is not _MISSING and default_factory is not
_MISSING and raise; otherwise proceed to call field(...) with the appropriate
argument and metadata.
In `@bakelite/tests/proto/test_serialization.py`:
- Around line 178-194: The test function rejects_fixed_array_wrong_size
currently builds an ArrayStruct via gen_code and test_struct but contains no
assertion; update it to either be removed or explicitly skipped/asserted: the
quick fix is to mark rejects_fixed_array_wrong_size with pytest.skip (or call
pytest.skip at start) with a brief message explaining that non-batched
fixed-size array validation is not implemented, referencing
ArrayStruct/gen_code/test_struct in the message; alternatively, if you want a
real test, replace the body with an explicit assertion about the packed output
or validation behavior (e.g., compare pack() output or expected exception) so
the test actually verifies a specific outcome.
---
Nitpick comments:
In `@bakelite/generator/cli.py`:
- Around line 20-27: Update the help string for the click option defined on
runtime_import (the click.option call with "--runtime-import") to clearly
describe the three behaviors: omitting the flag selects the relative import
"bakelite_runtime", passing the flag with no value selects the installed-package
import "bakelite.proto", and passing a value (e.g., --runtime-import=my.module)
uses that custom import path; modify the help text to succinctly list these
three modes and their resulting import values so users can understand the flag's
three behaviors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 113ba610-bdf5-4006-97ce-2b47852498ba
📒 Files selected for processing (9)
bakelite/generator/cli.pybakelite/generator/python.pybakelite/generator/templates/python.py.j2bakelite/proto/runtime.pybakelite/proto/serialization.pybakelite/tests/proto/test_serialization.pyexamples/arduino/Makefileexamples/arduino/bakelite.hexamples/arduino/proto.py
- Default runtime output to "bakelite.h" for cpptiny, "." for python - Remove unused `from enum import Enum` from generated code template - Use bounded read(4096) in sync poll to avoid blocking on serial ports - Remove bare except in async poll, let exceptions propagate - Add sleep(1ms) in message iterators to avoid CPU spin - Reject specifying both default and default_factory in bakelite_field - Mark array size validation test as skipped (not yet implemented)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bakelite/proto/serialization.py (1)
25-32: Consider renamingtypeparameter to avoid shadowing builtin.The parameter
typeshadows Python's built-intype(). While harmless within this function's scope, it's a minor anti-pattern that can confuse readers or cause issues if the builtin is needed later.♻️ Suggested rename
def bakelite_field( - type: str, + wire_type: str, *, max_length: int | None = None, array_size: int | None = None, default: Any = _MISSING, default_factory: Any = _MISSING, ) -> Any: """Define a bakelite field with serialization metadata. Args: - type: The bakelite wire type (e.g., "uint8", "int32", "string"). + wire_type: The bakelite wire type (e.g., "uint8", "int32", "string").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bakelite/proto/serialization.py` around lines 25 - 32, The parameter name "type" in the function bakelite_field shadows the builtin type()—rename the parameter (for example to "field_type") across the bakelite_field signature and every use inside that function and update any callers or references in the module (including type hints, default handling, and any code constructing bakelite_field) to use the new name to avoid confusion and preserve builtin access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bakelite/generator/cli.py`:
- Around line 20-27: The help text for the Click option runtime_import is
incorrect: it says "omit=runtime" but the actual default is "bakelite_runtime"
(the option is defined with flag_value="bakelite.proto" and default=None and
later resolved to "bakelite_runtime"). Update the help string for the Click
option (the --runtime-import definition / runtime_import flag) to reflect the
actual behavior, e.g. "Import path for runtime. No value=bakelite.proto,
omit=bakelite_runtime", or alternatively change the default resolution logic to
match "runtime" if you want the help text to remain "omit=runtime".
---
Nitpick comments:
In `@bakelite/proto/serialization.py`:
- Around line 25-32: The parameter name "type" in the function bakelite_field
shadows the builtin type()—rename the parameter (for example to "field_type")
across the bakelite_field signature and every use inside that function and
update any callers or references in the module (including type hints, default
handling, and any code constructing bakelite_field) to use the new name to avoid
confusion and preserve builtin access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9de46d37-d9f1-42d6-b568-8f6e8c055fc0
📒 Files selected for processing (5)
bakelite/generator/cli.pybakelite/generator/templates/python.py.j2bakelite/proto/runtime.pybakelite/proto/serialization.pybakelite/tests/proto/test_serialization.py
🚧 Files skipped from review as they are similar to previous changes (1)
- bakelite/tests/proto/test_serialization.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bakelite/generator/cli.py (1)
62-65:⚠️ Potential issue | 🟡 MinorHandle directory paths for cpptiny runtime output.
If
-opoints to a directory (for example-o .), this branch still callsopen()on a directory and crashes. Normalize directory inputs to a filename (e.g.,bakelite.h) before writing.Suggested patch
if language == "cpptiny": - output_path = output_path or "bakelite.h" + target = Path(output_path or "bakelite.h") + if target.is_dir(): + target = target / "bakelite.h" + target.parent.mkdir(parents=True, exist_ok=True) generated_file = cpptiny.runtime() - with open(output_path, "w", encoding="utf-8") as f: + with target.open("w", encoding="utf-8") as f: f.write(generated_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bakelite/generator/cli.py` around lines 62 - 65, The code currently writes cpptiny.runtime() to output_path without handling when output_path is a directory (e.g., "-o ."), causing open() to fail; update the write path before opening by checking if output_path is a directory (use os.path.isdir(output_path)) and, if so, replace it with os.path.join(output_path, "bakelite.h") (or the default filename) so generated_file from cpptiny.runtime() is written to a file, not a directory; ensure os is imported and keep using the variables output_path, generated_file, and the with open(...) write block as shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bakelite/generator/cli.py`:
- Around line 70-71: The loop that writes generated Python runtime files uses
(runtime_dir / filename).write_text(content) which is locale-dependent; update
the write to explicitly specify UTF-8 encoding by calling the write_text
equivalent with encoding='utf-8' (i.e., change the write_text call inside the
for filename, content in python.runtime().items() loop to pass encoding='utf-8')
so all runtime files are written deterministically across environments.
---
Duplicate comments:
In `@bakelite/generator/cli.py`:
- Around line 62-65: The code currently writes cpptiny.runtime() to output_path
without handling when output_path is a directory (e.g., "-o ."), causing open()
to fail; update the write path before opening by checking if output_path is a
directory (use os.path.isdir(output_path)) and, if so, replace it with
os.path.join(output_path, "bakelite.h") (or the default filename) so
generated_file from cpptiny.runtime() is written to a file, not a directory;
ensure os is imported and keep using the variables output_path, generated_file,
and the with open(...) write block as shown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ff1651d-5747-49ed-82d8-0f9c50f9352f
📒 Files selected for processing (1)
bakelite/generator/cli.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bakelite runtime -l pythoncommand to generate runtime files--runtime-importflag for custom import pathsasync_=TrueparameterBufferedIOBase(sync) andStreamReader/StreamWriter(async)messages()as the primary iterator interface for consuming streamsStruct,BakeliteEnum)struct.pack/unpackcallsNote: async support was folded into this PR because the codegen optimization
commit was written on top of it — they share changes to
runtime.py.Test plan
pixi run pytestpassesasyncio.open_connectionSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests