Conversation
|
/review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23a8f36718
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR replaces the ephemeral per-process analytics identifier with a persistent user ID stored under the Lightly Train cache directory, and updates/extends unit tests to validate the new behavior.
Changes:
- Introduce
_user_idand_load_user_id()to persist a UUID inuserid.txtunderEnv.LIGHTLY_TRAIN_CACHE_DIR. - Update event payload
distinct_idto use the persisted_user_idrather than a session UUID. - Expand tracker unit tests to cover file creation, reading an existing ID, and read/write error fallbacks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/lightly_train/_events/tracker.py |
Implements persistent user ID loading/creation and uses it as distinct_id for event tracking. |
tests/_events/test_tracker.py |
Updates existing assertions and adds tests for _load_user_id() file-based persistence and error fallbacks. |
Comments suppressed due to low confidence (1)
src/lightly_train/_events/tracker.py:154
track_eventinitializes_user_id(and may read/writeuserid.txt) before checkingLIGHTLY_TRAIN_EVENTS_DISABLED/ rate-limiting / queue-size early returns. This introduces a side effect (creating a persistent identifier file) even when analytics are disabled or the event is dropped. Consider moving the_user_idlazy-load to just before buildingevent_data, after all early-return checks (at least after the events-disabled guard).
global _last_flush, _user_id
if _user_id is None:
_user_id = _load_user_id()
current_time = time.time()
if Env.LIGHTLY_TRAIN_EVENTS_DISABLED.value or (
current_time - _last_event_time.get(event_name, -100.0) < _RATE_LIMIT_SECONDS
):
return
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b527e02a18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/review |
What has changed and why?
How has it been tested?
Did you update CHANGELOG.md?
Did you update the documentation?