chore: add skills, design docs, and tooling config#190
chore: add skills, design docs, and tooling config#190nv-alicheng wants to merge 1 commit intofeat/alicheng-test-cleanupfrom
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the development environment and documentation. It introduces new AI assistant skills to guide developers on efficient Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new documentation in the form of Claude/Cursor skills and system design documents, and significantly refactors the zmq_pubsub_demo.py script. The documentation is thorough and well-structured, providing valuable insights into the msgspec patterns and the pub-sub architecture. The demo script update is a great improvement, now demonstrating the out-of-process subscriber pattern with verification, which aligns well with the design documents. My review includes a couple of suggestions to enhance clarity in the documentation and improve resource management in the demo script.
|
|
||
| ## NDJSON with encode_into | ||
|
|
||
| For line-delimited JSON, use `encode_into()` with `buffer.extend()` to avoid copies: |
There was a problem hiding this comment.
The description for NDJSON says to use encode_into() with buffer.extend(), but the code example doesn't use buffer.extend(). This is confusing as encode_into and buffer.extend serve different purposes. The example shows writing a single encoded message to a file, which is a valid use case for encode_into to avoid intermediate allocations.
To improve clarity, I suggest updating the description to better match the example. For instance:
"For line-delimited JSON, use encode_into() to write into a reusable buffer before sending the data to a file or socket. This avoids extra memory copies."
| conn = sqlite3.connect(str(events_db)) | ||
| try: | ||
| cur = conn.execute( | ||
| "SELECT id, sample_uuid, event_type, timestamp_ns, data FROM events ORDER BY id" | ||
| ) | ||
| rows = cur.fetchall() | ||
| for row in rows: | ||
| row_id, sample_uuid, event_type, timestamp_ns, data = row | ||
| sample_short = ( | ||
| (sample_uuid[:8] + "..") if sample_uuid else "N/A" | ||
| ) | ||
| data_preview = ( | ||
| data[:60] + b"..." | ||
| if data and len(data) > 60 | ||
| else data or b"" | ||
| ) or b"" | ||
| logger.info( | ||
| " id=%s | event_type=%s | sample=%s | timestamp_ns=%s | data=%s", | ||
| row_id, | ||
| event_type, | ||
| sample_short, | ||
| timestamp_ns, | ||
| data_preview.decode("utf-8", errors="replace"), | ||
| ) | ||
| logger.info("Total rows: %s", len(rows)) | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
The SQLite connection is manually closed in a finally block. It's more idiomatic and safer to use the connection object as a context manager (with statement). This ensures the connection is properly closed even if errors occur, and it also handles transaction commits or rollbacks automatically.
try:
with sqlite3.connect(str(events_db)) as conn:
cur = conn.execute(
"SELECT id, sample_uuid, event_type, timestamp_ns, data FROM events ORDER BY id"
)
rows = cur.fetchall()
for row in rows:
row_id, sample_uuid, event_type, timestamp_ns, data = row
sample_short = (
(sample_uuid[:8] + "..") if sample_uuid else "N/A"
)
data_preview = (
data[:60] + b"..."
if data and len(data) > 60
else data or b""
) or b""
logger.info(
" id=%s | event_type=%s | sample=%s | timestamp_ns=%s | data=%s",
row_id,
event_type,
sample_short,
timestamp_ns,
data_preview.decode("utf-8", errors="replace"),
)
logger.info("Total rows: %s", len(rows))
except sqlite3.Error as e:
logger.error("Failed to read from SQLite DB: %s", e)- Add msgspec-patterns and msgspec-struct-gc-check Claude/Cursor skills - Add design docs for services architecture and metrics aggregator - Update .gitignore for new artifact patterns - Update zmq_pubsub_demo.py for new transport API
nv-alicheng
force-pushed
the
feat/alicheng-skills-and-docs
branch
from
683b17c to
8c5f45f
Compare
nv-alicheng
force-pushed
the
feat/alicheng-test-cleanup
branch
from
b723356 to
bc244a8
Compare
| @@ -0,0 +1,119 @@ | |||
| --- | |||
There was a problem hiding this comment.
Please fold it or symlink to claude skills to avoid duplication
nvzhihanj
left a comment
nvzhihanj
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Codex + Claude
Found 0 issues.
This PR adds skill definitions, design documentation, tooling config (.gitignore), a demo script refactor, and a minor test formatting fix. No source code logic changes — no issues to flag.
Review Council — Multi-AI Code Review CouncilReviewed by: Codex + Claude Found 0 issues across 8 files. This PR adds skill definitions (msgspec-patterns, msgspec-struct-gc-check), design documentation for the EventRecord pub-sub and metrics aggregator services, tooling config (.gitignore updates), a demo script refactor (subprocess-based event_logger with JSONL+SQL), and a minor test formatting fix. No source code logic changes — nothing to flag.
|
3 participants
What does this PR do?
Type of change
Related issues
Testing
Checklist