fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289)#535
fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289)#535mattmillerai wants to merge 2 commits into
Conversation
…dantic_core (BE-3289)
`mixpanel` 5.2.0 added a hard `pydantic` dependency for its feature-flags
module, so `from mixpanel import Mixpanel` at tracking.py import time pulled the
compiled `pydantic_core` extension into every `comfy` process and held it open
for the process lifetime.
`comfy install` shells out to `uv pip install` against its own interpreter's
environment. On Windows an open DLL cannot be replaced, so uv could not upgrade
pydantic_core:
error: failed to remove file `...\pydantic_core\_pydantic_core.cp312-win_amd64.pyd`:
Access is denied. (os error 5)
uv exited 2 having already unlinked the distribution metadata, leaving the
package half-removed, so the next `comfy` invocation in the same job died with
`ImportError: cannot import name '__version__' from 'pydantic_core' (unknown
location)`. mixpanel is unpinned and CI installs via `pip install -e .`, which is
why this began failing on every PR with no code change.
Defer the mixpanel/posthog imports and provider construction to the first send.
A run that never sends telemetry — no consent, or DO_NOT_TRACK — never loads the
extension at all, so uv is free to replace it. Behavior is unchanged: providers
are still built on first dispatch, just not at import.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
size:M
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟢 Low | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…BE-3289) Deferring the mixpanel/posthog imports moved them off startup and onto the user's command path, so the failure they used to hit at import can now surface mid-run. _get_providers() is called in _dispatch's loop header, outside the per-provider try/except that only wraps .track() — so an ImportError from a half-removed pydantic_core (exactly the state this PR exists to prevent, but still reachable from an earlier bad install) propagated through track_event and took the user's command down. Build each provider independently and cache the result, including []: one unusable SDK no longer silences the other, telemetry goes quiet instead of crashing, and a doomed import isn't retried on every later event. Also take _PROVIDERS_LOCK before reading PROVIDERS in the atexit flush, so an in-flight build resolves first rather than the hook seeing None mid-construction and exiting without draining a racing thread's PostHog queue. The lock is released before flushing so a slow network drain can't block a concurrent send. Both regression tests fail on the pre-fix tracking.py and pass here. Raised by cursor-review (5 of 8 reviewers). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 93f2d8d addressing both cursor-review findings (details on each thread). The failing
|
|
Correction to my comment above: no follow-up needed — the tomlkit fix is already in flight. My root-cause diagnosis landed independently on the same line, which is a good cross-check:
Both target So the |
|
Resolver pass: both review threads (the unguarded lazy construction and the The acceptance test passes. The remaining red I verified the cause rather than just asserting it is pre-existing. Amusingly this is the same class of bug this PR fixes (an unpinned dependency resolving fresh in CI), just via a different package. It blocks every PR in the repo, so it wants its own PR rather than being folded in here — filed as a follow-up. |
|
Correction to my note above: the tomlkit break is already being fixed — no new follow-up needed, and I did not file one. Existing coverage:
So nothing further is needed on #535 itself: its threads are resolved, its target job is green, and its only red is owned by the PRs above. Merging #537 (or either of #533/#536) should turn |
f91cab6 accidentally carried a 287-line uv.lock regeneration that has nothing to do with the HTTP oversize fix. The drift is real but pre-existing: #490 added the `bench` optional-dependency extra (anthropic>=0.40) to pyproject.toml without relocking, so main's lock has been stale since it merged — `uv lock --check` fails on main today. Any bare `uv run` in the repo silently re-locks and re-dirties the file, which is how it got picked up here. Re-locking is not this PR's job, and carrying it here invites conflicts with the dependency PRs already in flight (#533/#535/#536). No CI job consumes uv.lock, so reverting to main's copy is behavior-neutral. Filed as a follow-up instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CI status note (from a review sweep): the only failing check — `build` (pytest.yml, the full-suite job that installs deps unpinned) — is not caused by this PR. It is the repo-wide This PR itself is clean: no merge conflict, both Cursor review threads resolved in 93f2d8d, and its own tests ( |
1 participant
ELI-5
comfysaid hello to its analytics library on startup. That library quietlyloaded a compiled Windows DLL and never let go of it. Later in the same run,
comfy installaskeduvto upgrade that exact DLL — and Windows refuses tooverwrite a file that a running program has open.
uvgave up halfway, leavingthe package in pieces, and every
comfycommand after that crashed on import.Now we don't say hello to analytics until we actually have something to send. If
a run never sends telemetry, the DLL is never loaded, and
uvcan replace itfreely.
The failure
Every open PR has been failing the
test/ "Windows Specific Commands" check,unrelated to its own diff:
Root cause
The ticket hypothesized a half-installed dependency tree from the preceding
non-fatal
uv pip installexit-2. That's the symptom. The line the logtruncation was hiding is the actual cause — from
the failing job:
The chain:
mixpanel5.2.0 (unpinned; CI installs viapip install -e ., which does notread
uv.lock, so it resolves fresh every run) added a hardpydanticdependency for its feature-flags module. CI log:
Collecting pydantic<2.12,>=2.0.0 (from mixpanel->comfy-cli==0.0.0).comfy_cli/tracking.pyimportedmixpanelat module scope, so everycomfyprocess loaded_pydantic_core.cp312-win_amd64.pydand held it open.comfy installshells out touv pip install --requirement requirements.compiledagainst its own interpreter's environment. ComfyUI's pins need a different
pydantic-core, so uv must replace that
.pyd.Access is denied (os error 5)→ uv exits 2, having already unlinked the distribution metadata.pydantic_coreis now half-removed ("unknown location"), so the next command inthe step (
comfy --here launch) dies at import.This is why it started repo-wide with no code change, and why the ordinary
multi-platform Windows test job still passes — that job never runs
comfy install,so nothing ever tries to replace the loaded DLL.
Fix
Defer the
mixpanel/posthogimports and provider construction to the firstsend (
_get_providers(), called from_dispatch). All send paths are alreadygated on consent, so CI (
--skip-prompt, no consent) never constructs a providerand never loads the extension — leaving uv free to replace it.
This is the ticket's step 4, promoted from "resilience win" to the actual fix. It
also drops an SDK import from CLI startup.
Deliberately not pinning
mixpanel(the ticket's step 3): the pin would treatthe symptom, freeze us off analytics updates, and still leave the bug armed for any
future dependency that ships a compiled extension overlapping ComfyUI's pins. It
would also not help a consented user (see Known limitation).
Evidence
Empirical, on
import comfy_cli.cmdline(the exact chain in the traceback):pydantic_coreloaded into the process?origin/main_pydantic_core.cpython-314-darwin.sosys.modulesThe new
tests/comfy_cli/test_tracking_lazy_import.pyasserts this in freshsubprocesses (the pytest process has already imported too much to assert on
in-process). All 5 fail on
origin/main'stracking.pyand pass here — themechanism, not just my premise.
Known limitation (judgment call, flagged deliberately)
This fully fixes CI and any run without telemetry consent. A user with
telemetry enabled on Windows can still trip the same DLL lock if an event fires
before
comfy installreaches its uv subprocess. Fixing that properly means notinstalling into the interpreter's own environment (or sending telemetry
out-of-process) — a much larger change, well outside this ticket. Called out rather
than silently left: happy to file a follow-up.
Verification
ruff check .— passruff format --check .— passpytest— 2569 passed, 37 skipped. 9 pre-existing failures inregistry/test_config_parser.py+nodes/test_node_init.pyreproduceidentically on a clean
origin/maintree (verified by stashing this diff);they are unrelated to telemetry and not touched here.
test-windows.ymlrun — it triggers oncomfy_cli/**, which this diff touches.Note for review
tests/e2e/verify_tracking_live.pyiteratedtracking.PROVIDERSdirectly, which isnow
Noneuntil the first send. Switched to_get_providers(). E2E is opt-in(
TEST_E2E=true), so no suite run would have caught it._get_providers()is double-checked-locked: module-level construction wassingle-shot via the import lock, and a lazy global would otherwise let a racing
build strand a PostHog client — and its unflushed queue — in the list that lost.
Nothing tracks off-thread today (the
run_clipools spawn subprocesses); thispreserves a property the old code got for free rather than relying on that.