feat(odin): Add intermediate progress reporting and report_progress API to ActionContext#548
Draft
vikramlc-cognite wants to merge 4 commits into
Draft
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces intermediate progress reporting for log upload actions by adding a report_progress method to ActionContext and reporting progress as files complete uploading. Feedback was provided to throttle these progress updates (e.g., reporting at most 10 times) to prevent queue bloat and artificial delays when uploading a large number of files.
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
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
Adds
ActionContext.report_progress()as a clean API for mid-action progress updates, and wires it into the fetch_logs upload loop so Odin receives a running status update with "Uploading: N/M files complete" after each file resolves.Type of change
What changed
ActionContext.report_progress(message)- queues a running ActionUpdate with the given message via _checkin_worker, imported ActionStatus/ActionUpdate from _dto at module level_log_upload_action.py: Replaced the one-liner upload_results.extend(... for future in as_completed(...)) with an explicit loop; after each future resolves, callsctx.report_progress(f"Uploading: {completed}/{total} files complete")test_log_upload_action.py: 4 new tests - report_progress unit test (queues correct external_id, status, message), multi-file progress (2 files → both 1/2 and 2/2 messages present), single-file progress (1/1), and no-candidates case (zero progress updates, still succeeds)Why it changed
What to focus on during review
report_progressdenominator: Uses len(candidates) (files actually being uploaded), not num_days (requested date range). A 3-day request where 1 file is missing shows 1/2, 2/2 - not 1/3, 2/3. This is intentional: the denominator tracks upload progress, not date coverage. The final succeeded message already uses num_days for the overall picture.Test evidence
tests/test_unstable/test_log_upload_action.py41 passed (4 new)ruff check + ruff format - all clean
Risks and unknowns
Rollout and rollback
Checklist