Skip to content

feat(build): push reports how much it uploaded, how long it took and how it ended (BE-16046) - #928

Open
vqt123 wants to merge 1 commit into
mainfrom
vinh/be-16046-push-upload-event
Open

vqt123 wants to merge 1 commit into
mainfrom
vinh/be-16046-push-upload-event

Conversation

@vqt123

@vqt123 vqt123 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Description

TL;DR: comfy build push reports how much it uploaded, how long that took and how it ended, as one build:push_upload event.

Why: nothing measured an upload before this, in the CLI or the portal: not size, not seconds, not how it ended. The push already computes every number but the time (upload_count, upload_bytes, uploaded, deduped are in its --json payload today).

Before / After: after the uploads, one event with upload_count, upload_bytes, seconds, outcome, and on success uploaded and deduped (After). On a push that dies mid-upload the event still goes out with outcome: error and no guessed split. A push with nothing to upload sends nothing. No filename, ever: a private model's name is the customer's. Today: nothing (Before).

Rate is not sent; it is upload_bytes / seconds.

Part of BE-16046, whose portal half is https://github.com/Comfy-Org/platform/pull/1107.

How has this been tested?

pytest: 7858 passed (7861 on the tracker branch), 2 failed, 38 skipped. Both failures are not from this change: test_node_deps.py::test_non_pep440_installed_version_is_unknown_not_a_crash and test_http.py::test_an_unloadable_supplement_falls_through_to_the_platform_roots fail identically on an unmodified main checkout in a fresh environment (Python 3.12; CI targets 3.10). ruff check and ruff format --check clean. The "Test CLI Tool on GPU runners" job fails on every PR in this repo today (10 of the last 10 runs, five authors) on tests/e2e/test_e2e_uv_compile.py, which this does not touch.

Three new tests on the event: a finished push, a push that died mid-upload, a push with nothing to upload.

Not run: against real PostHog.

Documentation

  • Not user-facing.

Screenshots or screen capture

n/a

…how it ended (BE-16046)

Nothing measured an upload before this, in the CLI or the portal. One
build:push_upload event per push that had something to upload: count, bytes,
seconds, outcome, and on success how many were sent against how many the
builder already held. No filename; a private model's name is the customer's.
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 36e8e156-354e-4de5-8f64-151ea6584c48

📥 Commits

Reviewing files that changed from the base of the PR and between 64026d2 and eb0199c.

📒 Files selected for processing (2)
  • comfy_cli/command/build.py
  • tests/comfy_cli/command/test_build_push_event.py

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

build push now tracks upload duration, size, counts, and outcome. Failed uploads emit error events before re-raising. Completed uploads include uploaded and deduplicated counts. Empty uploads emit no event. Tests cover all three paths.

Changes

Push upload tracking

Layer / File(s) Summary
Upload tracking flow
comfy_cli/command/build.py, tests/comfy_cli/command/test_build_push_event.py
push_cmd measures upload_assets with time.monotonic() and records success or error events. _track_push_upload emits upload totals and rounded duration, omits events for empty uploads, and adds uploaded and deduped counts only after success. Tests cover completed, failed, and empty uploads.

Suggested reviewers: guill

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to eb019

This change adds upload telemetry while preserving push success and failure behavior. Focused tests cover the reported outcomes, so it is mergeable with normal checks.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
✨ Simplify code
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from guill September 22, 2026 13:12
@vqt123
vqt123 requested review from huntcsg, james00012 and wei-hai and removed request for annehe9 and huntcsg September 22, 2026 15:59

@wei-hai wei-hai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed upload success/error reporting, dedup counts, and omission of private filenames. No blocking findings. All three new event tests pass locally on Python 3.10.

The GPU CI job remains red: its log shows the two uv-compile conflict-message tests failing, the same failures across these four PRs in code outside these changes. Other reported checks pass. I did not rerun GPU E2E or live analytics/provider operations.

@james00012 james00012 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Reads as: a push sends one event with its upload count, bytes, seconds and outcome. Head eb0199c, judged as comfy-cli's owner.

Optional

  • Correctness: the derived upload rate is inflated whenever the builder already held a file comfy_cli/command/build.py:2143. The bytes count skipped files too; return the bytes actually sent.
  • Contract: a person pressing Ctrl-C counts as a failed upload comfy_cli/command/build.py:2006. The portal half, platform#1107, reports cancelled separately, so the two halves disagree.
  • Tests: the event can be unhooked from the push with every test green tests/comfy_cli/command/test_build_push_event.py:27. One push with a failing upload would prove the promised failure path.
  • Modularity: the upload counts are computed in three places comfy_cli/command/build.py:2138. The event and the JSON output will drift.
  • Verbosity: two comments record history comfy_cli/command/build.py:2132. The line that nothing measured uploads before goes stale on merge.

Clean: Scope, Resilience, Access, Reachability, Rollout.
Native review: 8 findings, folded into the list above.
Checked: removing both calls from the push left 636 build tests green; a real push sent the same counts as its JSON output.
Bots: the automated review missed the rate and the cancel.

@comfy-greenlight-bot

Copy link
Copy Markdown

Swarmhost agentic review

An internal review is in progress. Detailed evaluations are not published on public repositories.

Updated by Swarmhost's agentic review process.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants