Added PostHog telementry#254
Merged
Merged
Conversation
klemen1999
requested review from
conorsim,
kozlov721 and
tersekmatija
and removed request for
a team
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 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 |
Collaborator
Author
|
NOTE: Changes to requirements.txt, RVC2 Dockerfile and RVC3 Dockerfile are temporary just for the tests to pass. They should be reverted once related luxonis-ml branch gets merged and released |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelconverter/__main__.py (1)
185-223: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the original
main_stageinput for telemetry.Line 223 reports
main_stage_provided=Trueafter Line 185 fillsmain_stagefrom config defaults, so telemetry can’t distinguish user-provided input from resolved config state.Proposed fix
- main_stage = main_stage or _main_stage + main_stage_provided = main_stage is not None + main_stage = main_stage or _main_stage @@ - main_stage_provided=main_stage is not None, + main_stage_provided=main_stage_provided,🤖 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 `@modelconverter/__main__.py` around lines 185 - 223, The telemetry flag for main stage input is using the resolved `main_stage` value instead of the original caller input, so it cannot tell whether the user actually supplied a main stage. In `__main__.py`, preserve the initial `main_stage` argument before it is defaulted from `_main_stage`, and pass that original value into `build_conversion_summary` for `main_stage_provided` so `build_flow_properties` reflects user input accurately.
🧹 Nitpick comments (3)
docker/rvc2/Dockerfile (1)
110-119: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid shipping
gitin the final runtime image.
gitis only needed for the laterpip install -r ...steps that resolve VCS requirements. Installing it in its own layer keeps it in the published image permanently, which adds avoidable tool surface and image weight. Move the install into the same RUN as the pip installs and purge it before that layer commits.Proposed change
-RUN <<EOF - - set -e - - apt-get update - apt-get install -y --no-install-recommends git - rm -rf /var/lib/apt/lists/* - -EOF - ... RUN <<EOF set -e - pip install --upgrade pip --no-cache-dir + apt-get update + apt-get install -y --no-install-recommends git + pip install --upgrade pip --no-cache-dir pip install -r requirements.txt --no-cache-dir pip install -r requirements-rvc2.txt --no-cache-dir pip install openvino==2022.3.0 openvino-dev==2022.3.0 --no-cache-dir + apt-get purge -y --auto-remove git + rm -rf /var/lib/apt/lists/* EOF🤖 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 `@docker/rvc2/Dockerfile` around lines 110 - 119, The standalone apt-get install of git in the Dockerfile is leaving git baked into the final runtime image. Move the git installation into the same RUN block that performs the later pip install steps so it is only present while resolving VCS requirements, then remove git before that layer completes; use the existing Dockerfile RUN sequence around the pip install logic to keep the final image free of git.docker/rvc3/Dockerfile (1)
66-75: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid keeping
gitin the shipped RVC3 image.This looks build-time-only as well:
gitis needed for the following VCS-backedpip installcalls, not for steady-state runtime. Installing it in a separate layer leaves unnecessary tooling in the final image. Fold the install into the pip layer and purge it before the layer finishes.Proposed change
-RUN <<EOF - - set -e - - apt-get update - apt-get install -y --no-install-recommends git - rm -rf /var/lib/apt/lists/* - -EOF - ... RUN <<EOF set -e - pip install --upgrade pip --no-cache-dir + apt-get update + apt-get install -y --no-install-recommends git + pip install --upgrade pip --no-cache-dir pip install -r requirements.txt --no-cache-dir pip install -r requirements-rvc3.txt --no-cache-dir pip install openvino==2022.3.0 openvino-dev==2022.3.0 --no-cache-dir + apt-get purge -y --auto-remove git + rm -rf /var/lib/apt/lists/* EOF🤖 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 `@docker/rvc3/Dockerfile` around lines 66 - 75, The Dockerfile currently installs git in a standalone layer, which leaves build-only tooling in the final RVC3 image. Move the git install into the same layer that runs the VCS-backed pip installs, using the existing pip build step as the place to temporarily install git, and remove/purge it before that layer completes. Keep the change localized to the Dockerfile build flow around the pip install commands so git is not present in the shipped image.modelconverter/utils/telemetry.py (1)
352-356: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency: RVC3 branch skips
_drop_none.Unlike the RVC2/RVC4/Hailo branches, this dict is returned raw, so any
None(e.g.compress_to_fp16) leaks into the payload. Wrap it in_drop_nonefor consistency.🤖 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 `@modelconverter/utils/telemetry.py` around lines 352 - 356, The RVC3Config branch in telemetry serialization returns a raw dict, unlike the RVC2/RVC4/Hailo paths, so None values such as compress_to_fp16 can leak into the payload. Update the RVC3 handling in the telemetry helper to pass the returned dict through _drop_none, keeping behavior consistent with the other branches in the same function.
🤖 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 `@modelconverter/__main__.py`:
- Line 182: Result telemetry is being skipped for pre-conversion failures
because path validation happens before the try/finally path in __main__.py and
the RESULT_EVENT gate later ignores config/setup errors. Move the path
normalization/validation logic into the main try flow around the conversion
entrypoint, and update the RESULT_EVENT payload assembly to use fallback values
when conversion_start or conversion_summary is missing so failures still emit a
result event. Use the existing conversion_summary, conversion_start, and
phase="configuration" handling to keep the telemetry shape consistent.
- Around line 241-243: The archive name extraction in the archive-handling block
is truncating dotted names at the first period, so stable names like
my.model.nn_archive become my. Update the logic in the code that sets
archive_name near is_nn_archive(original_path) to remove only the archive suffix
from Path(original_path).name instead of splitting on the first dot, preserving
any dots that are part of the base name.
- Around line 805-839: The launcher path in __main__.py leaves the conversion
run ID set in os.environ after calling get_conversion_run_id(), which can leak
into later in-process launches. Update the try/finally flow around
run_in_configured_environment() to restore or clear the environment state after
the command finishes, using the existing conversion_run_id/get_conversion_run_id
and command_telemetry capture block as the reference point.
In `@modelconverter/utils/telemetry.py`:
- Around line 235-254: `command_result_from_exception` and
`command_failure_reason_from_exception` are misclassifying `SystemExit(0)` from
the launcher path as a failure. Update the exception handling in
`modelconverter/utils/telemetry.py` so `SystemExit` with code 0 (and still
None/130) is treated like success: return "success" from
`command_result_from_exception` and `None` from
`command_failure_reason_from_exception`. Keep the existing
`KeyboardInterrupt`/interrupt behavior unchanged and make sure
`command_result_from_exception` and `command_failure_reason_from_exception` stay
consistent for the `COMMAND_EVENT` telemetry.
In `@requirements.txt`:
- Around line 2-3: The luxonis-ml dependency is currently pinned to a mutable
Git branch ref, which makes installs non-reproducible. Update the requirements
entry to use an immutable commit SHA for the
git+https://github.com/luxonis/luxonis-ml.git reference, or revert the
luxonis-ml[data,nn_archive,s3,gcs,telemetry] specifier back to the published
~=0.9.0 release once the telemetry changes are included. Make sure the final
dependency string in requirements.txt points to a fixed revision rather than
feat/telemetry.
In `@tests/test_utils/test_telemetry.py`:
- Around line 182-185: The telemetry environment-reset test is still inheriting
LUXONIS_TELEMETRY_ENABLED from the workflow, so it is not testing the true
default path. In the test setup that clears LUXONIS_TELEMETRY_BACKEND,
LUXONIS_TELEMETRY_API_KEY, LUXONIS_TELEMETRY_ENDPOINT, and
LUXONIS_TELEMETRY_DEBUG, also clear LUXONIS_TELEMETRY_ENABLED with the same
monkeypatch.delenv pattern so the test exercises the intended default
configuration.
---
Outside diff comments:
In `@modelconverter/__main__.py`:
- Around line 185-223: The telemetry flag for main stage input is using the
resolved `main_stage` value instead of the original caller input, so it cannot
tell whether the user actually supplied a main stage. In `__main__.py`, preserve
the initial `main_stage` argument before it is defaulted from `_main_stage`, and
pass that original value into `build_conversion_summary` for
`main_stage_provided` so `build_flow_properties` reflects user input accurately.
---
Nitpick comments:
In `@docker/rvc2/Dockerfile`:
- Around line 110-119: The standalone apt-get install of git in the Dockerfile
is leaving git baked into the final runtime image. Move the git installation
into the same RUN block that performs the later pip install steps so it is only
present while resolving VCS requirements, then remove git before that layer
completes; use the existing Dockerfile RUN sequence around the pip install logic
to keep the final image free of git.
In `@docker/rvc3/Dockerfile`:
- Around line 66-75: The Dockerfile currently installs git in a standalone
layer, which leaves build-only tooling in the final RVC3 image. Move the git
install into the same layer that runs the VCS-backed pip installs, using the
existing pip build step as the place to temporarily install git, and
remove/purge it before that layer completes. Keep the change localized to the
Dockerfile build flow around the pip install commands so git is not present in
the shipped image.
In `@modelconverter/utils/telemetry.py`:
- Around line 352-356: The RVC3Config branch in telemetry serialization returns
a raw dict, unlike the RVC2/RVC4/Hailo paths, so None values such as
compress_to_fp16 can leak into the payload. Update the RVC3 handling in the
telemetry helper to pass the returned dict through _drop_none, keeping behavior
consistent with the other branches in the same function.
🪄 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: edc78f15-3703-4ef6-8764-6f6408fdadb5
📒 Files selected for processing (24)
.github/workflows/actions_autoupdate.yaml.github/workflows/bom-test.yaml.github/workflows/hailo_publish.yaml.github/workflows/hailo_test.yaml.github/workflows/modelconverter_test.yaml.github/workflows/publish.yaml.github/workflows/python-publish.yml.github/workflows/rvc2_publish.yaml.github/workflows/rvc2_test.yaml.github/workflows/rvc3_publish.yaml.github/workflows/rvc3_test.yaml.github/workflows/rvc4_publish.yaml.github/workflows/rvc4_test.yaml.github/workflows/semgrep.yaml.github/workflows/unittests.yamldocker/rvc2/Dockerfiledocker/rvc3/Dockerfilemodelconverter/__main__.pymodelconverter/utils/__init__.pymodelconverter/utils/docker_utils.pymodelconverter/utils/target_versions.pymodelconverter/utils/telemetry.pyrequirements.txttests/test_utils/test_telemetry.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
2 participants
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.
Purpose
Adds end-to-end telemetry for
modelconverter convert, split into three events:modelconverter/__main__.pymodelconverter/__main__.pyfinallyblock with duration, upload status, artifact count, and peak RAM inmodelconverter/__main__.pyIntroduces a dedicated telemetry helper module in
modelconverter/utils/telemetry.py:luxonis_ml.telemetryMODELCONVERTER_CONVERSION_RUN_IDuser_interrupt,config_error,upload_error,conversion_error, or generic runtime failureWires telemetry across Docker boundaries:
docker_exec()now forwards telemetry-related environment variables into the compose config inmodelconverter/utils/docker_utils.pylauncher()skips duplicate outer event capture when already running inside Docker, so one conversion run keeps one shared run id and cleaner telemetry inmodelconverter/__main__.pyRefactors target version lookup into a standalone helper in
modelconverter/utils/target_versions.py, then re-exports it throughmodelconverter/utils/__init__.py.Updates dependency requirements in
requirements.txt:luxonis-mlfrom>=0.8.5to~=0.9.0telemetryextrann-archiveextra naming tonn_archiveDisables telemetry in CI by setting
LUXONIS_TELEMETRY_ENABLED: "false"across workflow files under.github/workflows, which keeps tests/publish jobs from emitting telemetry.Adds focused telemetry tests in
tests/test_utils/test_telemetry.py:Example of the new event shape:
{ "flow_name": "modelconverter_conversion_lifecycle", "conversion_run_id": "...", "flow_step": "result_recorded", "target": "rvc4", "config_source": "direct_model_input", "result": "success", "duration_ms": 1234, "uploaded_output": True, "peak_ram_usage_bucket": "1g_4g", }NOTES:
--devflag so docker image gets rebuilt and actually uses the telementry luxonis-ml branchSpecification
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
gitwhere needed.luxonis-mlto use the repository version with telemetry extras.