Feat/telemetry - #223
Conversation
📝 WalkthroughWalkthroughThe CLI now uses cyclopts, resolves exporters through a conversion registry, and records telemetry across validation, export, archive creation, and remote upload phases. Typed telemetry utilities classify outcomes and failure reasons, with tests covering lifecycle events and version handling. ChangesConversion CLI and telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as convert
participant Telemetry as telemetry helpers
participant Exporter as exporter registry
participant Remote as remote upload
CLI->>Telemetry: start conversion run
CLI->>Exporter: create exporter and run exports
Exporter-->>CLI: return archive path
CLI->>Remote: upload archive when configured
Remote-->>CLI: return upload status
CLI->>Telemetry: emit result and command events
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tools/utils/config.py (1)
48-51: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer generator expressions over list comprehensions inside
any().Using a generator expression inside
any()avoids creating an intermediate list in memory and allows the operation to short-circuit as soon as the first matching condition is met.♻️ Proposed refactor
- if any([v <= 0 for v in value]): + if any(v <= 0 for v in value): raise ValueError("Image size values must be greater than 0.") - if any([v % 32 != 0 for v in value]): + if any(v % 32 != 0 for v in value): raise ValueError("Image size values must be divisible by 32.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/utils/config.py` around lines 48 - 51, In the value validation logic, update both any() calls to consume generator expressions directly instead of list comprehensions, preserving the existing positivity and divisibility checks and error messages.tools/utils/telemetry.py (2)
46-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
get_exporter_familyhas no fallback for unmapped versions.
EXPORTER_FAMILIES[version]currently matches every entry inYOLO_VERSIONS(main.py), but if a new version is ever added toYOLO_VERSIONS/the exporter elif-chain without a matching entry here, this raises an unhandledKeyErrorinstead of a clean, telemetry-friendly failure — unlike the exporter-selection elif-chain inmain.py, which has a defensiveelsebranch for unrecognized versions.♻️ Add a defensive fallback
def get_exporter_family(version: str) -> str: """Return the sanitized exporter family for an effective version.""" - return EXPORTER_FAMILIES[version] + return EXPORTER_FAMILIES.get(version, "unknown")Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/utils/telemetry.py` around lines 46 - 62, Update get_exporter_family to handle versions absent from EXPORTER_FAMILIES with a defensive, telemetry-friendly fallback instead of allowing a KeyError. Preserve the existing mapped-family behavior and align the fallback with the unrecognized-version handling used by the exporter-selection chain in main.py.
201-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
command_failure_reason_from_stateandresult_failure_reason_from_stateare identical.Both functions have byte-for-byte identical bodies. Consider consolidating into a single function (or making one an alias of the other) to avoid the two diverging accidentally in the future.
♻️ Consolidate the duplicate mapping functions
-def command_failure_reason_from_state( - *, - phase: str, - exc: BaseException | None, -) -> str | None: - """Map an exception/phase pair to a coarse command failure reason.""" - result = command_result_from_exception(exc) - if result == "success": - return None - if result == "interrupted": - return "user_interrupt" - - return _failure_reason_from_state(phase=phase, exc=exc) - - -def result_failure_reason_from_state( - *, - phase: str, - exc: BaseException | None, -) -> str | None: - """Map an exception/phase pair to a conversion-result failure reason.""" - result = command_result_from_exception(exc) - if result == "success": - return None - if result == "interrupted": - return "user_interrupt" - - return _failure_reason_from_state(phase=phase, exc=exc) +def failure_reason_from_state( + *, + phase: str, + exc: BaseException | None, +) -> str | None: + """Map an exception/phase pair to a coarse failure reason.""" + result = command_result_from_exception(exc) + if result == "success": + return None + if result == "interrupted": + return "user_interrupt" + + return _failure_reason_from_state(phase=phase, exc=exc) + + +command_failure_reason_from_state = failure_reason_from_state +result_failure_reason_from_state = failure_reason_from_state🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/utils/telemetry.py` around lines 201 - 228, Consolidate the identical logic in command_failure_reason_from_state and result_failure_reason_from_state by making one reuse the other or extracting their shared implementation into a single helper. Preserve both public function names and their current success, interruption, and fallback mappings while eliminating duplicate bodies.tests/test_telemetry.py (1)
198-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding end-to-end tests for the
onnx_export/nn_archive_exportfailure phases.
exporter_creation(SystemExit 4) andupload(SystemExit 7) failures are exercised end-to-end viamain_module.convert(...), butonnx_export(5) andnn_archive_export(6) are only covered indirectly through the abstract mapping test. A sibling test following the sameDummyExporter/BrokenExporterpattern (raising fromexport_onnx/export_nn_archive) would close this gap cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_telemetry.py` around lines 198 - 243, Add end-to-end tests alongside test_convert_emits_result_event_when_exporter_creation_fails that invoke main_module.convert with a DummyExporter raising from export_onnx and export_nn_archive. Assert SystemExit codes 5 and 6 respectively, and verify telemetry emits CONFIGURED_EVENT, RESULT_EVENT, and COMMAND_EVENT with failed results and the corresponding phase-specific failure_reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/main.py`:
- Around line 173-199: Wrap Config.get_config, resolve_path, and detect_version
in the same phase-specific exception handling used by the later export and
upload stages. For each operation, set the appropriate phase, catch its expected
failure, log a concise user-facing error with logger.error, and terminate with
the established defined exit code for that phase while preserving the existing
version validation behavior.
- Around line 207-217: Update the telemetry.capture calls in the
configuration-resolved flow and the corresponding cleanup/finally flow to catch
and suppress telemetry exceptions, ensuring telemetry remains best-effort.
Preserve the conversion result and any original conversion exception, and
continue setting phase to "configuration_resolved" after a capture attempt.
---
Nitpick comments:
In `@tests/test_telemetry.py`:
- Around line 198-243: Add end-to-end tests alongside
test_convert_emits_result_event_when_exporter_creation_fails that invoke
main_module.convert with a DummyExporter raising from export_onnx and
export_nn_archive. Assert SystemExit codes 5 and 6 respectively, and verify
telemetry emits CONFIGURED_EVENT, RESULT_EVENT, and COMMAND_EVENT with failed
results and the corresponding phase-specific failure_reason.
In `@tools/utils/config.py`:
- Around line 48-51: In the value validation logic, update both any() calls to
consume generator expressions directly instead of list comprehensions,
preserving the existing positivity and divisibility checks and error messages.
In `@tools/utils/telemetry.py`:
- Around line 46-62: Update get_exporter_family to handle versions absent from
EXPORTER_FAMILIES with a defensive, telemetry-friendly fallback instead of
allowing a KeyError. Preserve the existing mapped-family behavior and align the
fallback with the unrecognized-version handling used by the exporter-selection
chain in main.py.
- Around line 201-228: Consolidate the identical logic in
command_failure_reason_from_state and result_failure_reason_from_state by making
one reuse the other or extracting their shared implementation into a single
helper. Preserve both public function names and their current success,
interruption, and fallback mappings while eliminating duplicate bodies.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 410c3426-d73d-457f-96f2-a4455e106585
📒 Files selected for processing (19)
README.mdrequirements.txttests/conftest.pytests/test_telemetry.pytests/test_unittests.pytools/main.pytools/modules/backbones.pytools/modules/exporter.pytools/modules/heads.pytools/utils/config.pytools/utils/constants.pytools/utils/filesystem_utils.pytools/utils/in_channels.pytools/utils/telemetry.pytools/version_detection/version_detection.pytools/yolo/yolov10_exporter.pytools/yolo/yolov5_exporter.pytools/yolo/yolov8_exporter.pytools/yolov7/yolov7_exporter.py
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/utils/telemetry.py (1)
107-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the blind exception catch.
Catching
Exceptionblindly can inadvertently mask unexpected issues (such asTypeErrororValueErrordue to corrupted environment metadata).importlib.metadata.versionraisesPackageNotFoundErrorwhen a package is not found, which is already handled above.♻️ Proposed refactor
except PackageNotFoundError: return None - except Exception: - return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/utils/telemetry.py` around lines 107 - 108, Remove the broad `except Exception` handler surrounding the `importlib.metadata.version` lookup in the relevant telemetry helper, leaving the existing `PackageNotFoundError` handling intact so unexpected `TypeError` and `ValueError` exceptions propagate.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tools/utils/telemetry.py`:
- Around line 107-108: Remove the broad `except Exception` handler surrounding
the `importlib.metadata.version` lookup in the relevant telemetry helper,
leaving the existing `PackageNotFoundError` handling intact so unexpected
`TypeError` and `ValueError` exceptions propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 362fd6b4-dd61-4175-9209-4fabf1aa8299
📒 Files selected for processing (3)
tests/test_telemetry.pytools/main.pytools/utils/telemetry.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_telemetry.py
- tools/main.py
Test Results 6 files 6 suites 26m 25s ⏱️ Results for commit 9408e41. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/utils/telemetry.py (1)
225-226: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the “requested” flags with execution truthiness.
An empty URL is recorded as requested here, but
tools/main.pyLine 241 skips uploading because it checks truthiness. This yieldsremote_upload_requested=Truewithremote_upload_attempted=False.Proposed fix
- "remote_upload_requested": config.output_remote_url is not None, - "upload_plugin_override_provided": config.put_file_plugin is not None, + "remote_upload_requested": bool(config.output_remote_url), + "upload_plugin_override_provided": bool(config.put_file_plugin),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/utils/telemetry.py` around lines 225 - 226, Update the remote upload request flag in the telemetry construction to use the same truthiness condition as the upload execution check in main, so an empty output_remote_url records remote_upload_requested as false. Keep upload_plugin_override_provided unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/utils/telemetry.py`:
- Around line 225-226: Update the remote upload request flag in the telemetry
construction to use the same truthiness condition as the upload execution check
in main, so an empty output_remote_url records remote_upload_requested as false.
Keep upload_plugin_override_provided unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7371fef9-a850-4624-abec-caa106dcc74b
📒 Files selected for processing (4)
tests/test_telemetry.pytools/conversion_registry.pytools/main.pytools/utils/telemetry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_telemetry.py
2 participants
Purpose
configured,result, andcommandevents for PostHog vialuxonis-ml.typertocyclopts, makesconvertthe default command, refreshes help/docs in [README.md](https://github.com/luxonis/tools/blob/feat/telemetry/README.md), and tightens argument handling such as flexible--imgszparsing and lowercase--encodingsupport.luxonis-ml[telemetry]~=0.9.0, replacestyperwithcyclopts, and pinsgcsfs/s3fs.Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Submitted code was reviewed by a human: YES/NO
The author is taking the responsibility for the contribution: YES/NO
Summary by CodeRabbit
imgszparsing and comma-separated class-name handling.