Skip to content

Add model type and input-resolution usage telemetry - #2782

Open
SolomonLake wants to merge 7 commits into
mainfrom
model-telemetry
Open

Add model type and input-resolution usage telemetry#2782
SolomonLake wants to merge 7 commits into
mainfrom
model-telemetry

Conversation

@SolomonLake

@SolomonLake SolomonLake commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Extends usage telemetry so each model call can be attributed by Roboflow modelType (for example rfdetr-seg-nano) and by the post-preprocess / fixed model input resolution, bucketed in megapixels. Today usage rows identify a database resource_id but not the architecture/size, and they do not record the resolution the model actually ran at — which makes per-model and per-resolution analysis incomplete, especially for streams.

Model rows now include resource_details.model_type from a process-local cache populated when the registry resolves a model type at load time (so the usage decorator never does a registry/network lookup on the hot path). They also accumulate megapixel_buckets keyed by ranges such as 0.25-0.5, storing summed processed_frames and execution_duration per bucket. Fixed-input models use their configured size; dynamic models publish measured post-preprocess size via a ContextVar so concurrent infer() calls stay isolated. Frame counts come from the request image list rather than the padded tensor batch. SAM entrypoints that decorate infer_from_request publish encoder size explicitly. Families that override infer() without calling super() (doctr, grounding_dino, yolo_world, owlv2, including inference_models adapters) now decorate their own infer() so they emit model-category telemetry instead of being missing from per-model coverage.

Main elements:

  • inference/usage_tracking/megapixel_buckets.py — bucketing, measured-input context, fixed-size resolution
  • inference/usage_tracking/model_types.py — process-local model id → modelType map
  • inference/usage_tracking/decorator_helpers.py / collector.py / payload_helpers.py — extract, record, and merge buckets
  • Registry + base model / SAM / foundation-model infer paths that publish or decorate usage
  • Unit tests under tests/inference/unit_tests/usage_tracking/

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Other:

Testing

Unit tests

  • Megapixel bucket boundaries, merge, unknown-bucket reconciliation, ContextVar isolation
  • Decorator records fixed-input buckets, does not count batch padding, reads recorded model type without calling the registry
  • Registry records model type into the process-local map at resolve time
  • Guard that doctr / grounding_dino / yolo_world / owlv2 (legacy + adapters) decorate infer() for model telemetry
  • OwlV2 (height, width) and wrapped-backend fixed-size resolution

Integration tests

  • None for this PR.

Other

  • uv run pytest tests/inference/unit_tests/usage_tracking/ -q
  • uv run pytest tests/inference/unit_tests/models/ -q
  • uv run black / uv run isort on touched paths

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in hard-to-understand areas
  • My changes generate no new warnings or errors
  • I have updated the documentation accordingly (if applicable)

Additional Context

  • New payload field: megapixel_buckets. model_type is nested under existing resource_details and needs no new top-level column.
  • Platform /usage/inference currently rebuilds payloads from an allowlist and will drop megapixel_buckets until that handler (and the BigQuery inference_usage schema) accept the field. model_type already survives via resource_details.
  • doctr / grounding_dino / yolo_world may land in the unknown megapixel bucket when native arrays are passed into libraries that resize internally; OwlV2 reports a real fixed input size.
  • Direct self-hosted calls to these previously undecorated model entrypoints will now emit category="model" telemetry rows (workflow-routed calls were already covered by workflow rows).

Required follow-up: Redis / Dragonfly usage offloader

Serverless paths with REDIS_HOST store-and-forget into a UsageCollector sorted set; an external offloader (aws_lambdas/inference-usage-collector-redis-offload, also deployed as the Crusoe usage-collector CronJob against Dragonfly) drains and zip_usage_payloads-merges those rows. It loads payload_helpers.py from a pinned inference wheel (currently 0.49.5), which does not sum megapixel_buckets — conflicting merges last-writer-win instead.

This is required after this PR ships (in-process collector merge alone is not enough for Redis/Dragonfly traffic):

  1. Release an inference version that includes this PR’s merge_megapixel_buckets / merge_usage_dicts changes.
  2. In aws_lambdas/inference-usage-collector-redis-offload/Dockerfile, bump inference==… to that release.
  3. Bump src/version.py, rebuild/push the image (GAR for Crusoe CronJob; ECR if the AWS Lambda path is still live).
  4. Point the CronJob / Lambda at the new image tag (inference_usage_collector_redis_offload_version in tfvars if applicable) and deploy.

Until that pin moves, Redis/Dragonfly zip-merge undercounts or overwrites megapixel_buckets. Non-Redis paths (local / SQLite) are fine once this PR is deployed.

Made with Cursor

SolomonLake and others added 3 commits August 10, 2026 19:47
Decorate doctr, grounding_dino, yolo_world, and owlv2 infer paths so they
emit model-category telemetry rows, and resolve OwlV2 (h, w) input sizes.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
Contributor

👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely.

🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits.

Warning

💸 The Claude reviewer bills in credits, not vibes

Automated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared.

Draft when unsure, Ready when you mean it:

  • 🌱 Not sure the PR is in good shape yet? Keep it (or set it back) as a draft — drafts pause review, so you can push and iterate without burning credits on a moving target.
  • 💪 Feel strong about the contents? Mark it Ready for review and the reviewer will take a look.

However you get there, arrive prepared:

  • 🧱 Bring a SOLID, thorough PR. Point your local agent at our skills/ to tune it to our guidelines first — or, if you are one of those fabled carbon-based contributors, read them yourself. A half-baked diff costs exactly the same to review as a finished one.
  • Resolve every comment before you re-request review. Re-requesting with threads still open means paying twice for the same conversation.
  • 🔁 Do not use CI review as an inner loop for a local agent. The reviewer is not a step-by-step debugger — do the unfolding locally and arrive with the answer, not the search.
  • 🙋 If something looks off, ask a human. One question to a maintainer is cheaper and faster than three rounds of agent re-review chasing a misread.

Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.

  • Prefer to skip automated review entirely? Add the skip-claude-review label.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 7422b58d5f7be1320de816c298a5929f4b1402d8.

New commits are not auto-reviewed. Add the claude-review label (remove & re-add it to trigger again) when you want another review.

@github-actions

Copy link
Copy Markdown
Contributor

@SolomonLake — thanks for this. The telemetry plumbing is carefully built (ContextVar isolation across worker threads, bounded process-local model-type map off the hot path, batch-padding excluded from frame counts, adapter/backend input-size resolution) and the test coverage is strong. I found no hard blocker, but two questions materially affect the outcome and I can't resolve either from this repo alone.

Warning

This PR is on hold pending your answers. The review will not advance to sign-off and the PR should not proceed to merge until the IMPORTANT questions below are answered.

IMPORTANT — please answer

  1. Model-category processed_frames now counts batch size instead of 1 per call. Before this PR, _extract_usage_params_from_func_kwargs returned no frames key, so every model infer() recorded frames=1 (the record_usage default) and processed_frames incremented by 1 per call. This PR adds "frames": count_inference_images(image) (collector.py; decorator_helpers.get_model_frames_and_input_hw), so a batched call with a list of N images now increments processed_frames by N — and these rows carry billable=True. Your own tests assert this (test_record_usage_accumulates_megapixel_buckets, test_model_decorator_records_fixed_input_megapixel_buckets), so it looks intentional. Is this batch-counting change intended, and has it been coordinated with the usage-ingestion/billing side so it does not (a) change existing quota/billing math or (b) double-count against workflow-/request-category rows for the same inference?

  2. New payload fields are sent to the usage endpoint. megapixel_buckets, frames, and resource_details.model_type are now added to the emitted usage payload (empty_usage_dict, _update_usage_payload, get_model_resource_details_from_kwargs). Please confirm the ingestion endpoint accepts/ignores these new keys (strict validation won't reject the whole payload and silently drop usage). Also: merge_megapixel_buckets's docstring says it is "kept local to this module so the redis usage offloader can mirror it" — is that separate offloader actually updated to sum megapixel_buckets, or will offloaded rows drop the field on merge?

Notes (non-blocking)

  • No changelog.md entry is required here — inference/usage_tracking/** is neither the Execution Engine nor the inference_models versioned subsystem.
  • Adding model_type to resource_details can fragment a model's rows if some infer() calls happen before get_model_type() has recorded the type; in practice model loading resolves the type before the first inference, so this is informational only.

Re-review is not automatic: once you've answered and/or pushed changes, add the claude-review label (remove and re-add to re-trigger) to request a fresh pass. Unanswered IMPORTANT questions may keep this PR out of a release.

Reviewed at HEAD: 7422b58

last_request can be overwritten by concurrent empty-query POSTs from the
usage sender, which made model-type tests flake under full CI.

Co-authored-by: Cursor <cursoragent@cursor.com>
@SolomonLake

Copy link
Copy Markdown
Contributor Author

Is this batch-counting change intended, and has it been coordinated with the usage-ingestion/billing side so it does not (a) change existing quota/billing math or (b) double-count against workflow-/request-category rows for the same inference?

Yes, this is intentional. And I have ensured this will not result in double counting.

Please confirm the ingestion endpoint accepts/ignores these new keys (strict validation won't reject the whole payload and silently drop usage). Also: merge_megapixel_buckets's docstring says it is "kept local to this module so the redis usage offloader can mirror it" — is that separate offloader actually updated to sum megapixel_buckets, or will offloaded rows drop the field on merge?

Yes, the ingestion endpoint has been updated to accept these fields. And that separate offloader will be updated after these changes are merged.

@SolomonLake SolomonLake added the claude-review Use to trigger AI review manually label Aug 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 642f6213eb64cf09706cc6480b46aeff0ab5d683.

New commits are not auto-reviewed. Add the claude-review label (remove & re-add it to trigger again) when you want another review.

@github-actions

Copy link
Copy Markdown
Contributor

@SolomonLake — thanks for the answers, and for the follow-up commit hardening the roboflow-API query assertions against background usage POSTs (_assert_request_query_seen). That test change is sound and I have no findings on it.

Warning

This PR is still on hold pending one clarification. The review will not advance to sign-off until the billing/quota point below is confirmed. New commits since the last review were the test-hardening commit and a no-op merge of main, so no substantive code changed.

Status of the earlier IMPORTANT questions

  • Payload fields / offloader (Q2) — Resolved. You confirmed the ingestion endpoint accepts the new keys, and the Redis/Dragonfly offloader follow-up is documented in the PR description as required post-merge work. Consistent with the diff; nothing further needed in this repo.
  • Batch-count on billable model rows (Q1) — Still open, and I can only verify the in-repo half. I confirmed against code that model-category rows are billable=True by default (collector.py _is_billable, since get_model_resource_details_from_kwargs never sets billable), and that this PR (a) changes processed_frames on those rows from 1-per-call to N-per-call (get_model_frames_and_input_hw -> count_inference_images), and (b) newly emits billable category=model rows for doctr / grounding_dino / yolo_world / owlv2 entrypoints that previously emitted nothing. Whether that alters existing quota/billing math or double-counts against the workflow/request rows for the same inference lives entirely on the platform aggregation side, which is outside this repo — so your "intentional / ensured no double counting" assurance is not something I can confirm here.

IMPORTANT — please confirm (needs a concrete answer, ideally with maintainer sign-off)

On the platform billing/quota side, are category=model rows excluded from the billable aggregation (so only workflow/request rows are billed), or has the aggregation been updated to account for these newly-emitted model rows and the 1->N processed_frames change? A one-line description of how double-counting is avoided (which categories are summed for billing) would resolve this. This is the only remaining item.

Escalation to code-owners

@PawelPeczek-Roboflow @grzegorz-roboflow @dkosowski87 — this PR changes what is recorded on billable usage rows: processed_frames on category=model rows moves from 1-per-call to N-per-call (batch size), and four previously-undecorated foundation-model entrypoints (doctr, grounding_dino, yolo_world, owlv2, incl. inference_models adapters) now emit billable model rows. Those rows default to billable=True (collector.py _is_billable). The author confirms this is intentional and coordinated with the billing side, but that coordination cannot be verified from this repo. Please confirm the ingestion/quota aggregation treats these rows as intended (no quota-math change / no double-count vs workflow rows) before this lands in a release. Everything else in the PR is well-built and I found no hard code blocker.

Non-blocking notes (carried forward, unchanged)

  • No changelog entry required — inference/usage_tracking/** is neither the Execution Engine nor the inference_models versioned subsystem, and no version constant/lock-step pin is touched.
  • Adding model_type to resource_details can fragment a model rows if any infer() runs before get_model_type() has recorded the type; in practice loading resolves the type first, so this is informational.

Re-review is not automatic: once you have answered and/or pushed changes, add the claude-review label (remove and re-add to re-trigger) to request a fresh pass. Unanswered IMPORTANT questions may keep this PR out of a release.

Reviewed at HEAD: 642f621

@SolomonLake

Copy link
Copy Markdown
Contributor Author

On the platform billing/quota side, are category=model rows excluded from the billable aggregation (so only workflow/request rows are billed), or has the aggregation been updated to account for these newly-emitted model rows and the 1->N processed_frames change? A one-line description of how double-counting is avoided (which categories are summed for billing) would resolve this. This is the only remaining item.

On our Cloud APIs we do not bill for processed frames for workflows AND model lines. This would result in double billing. For legacy plans that bill on a per image basis, we only look at request rows. For usage credit based plans we only look at execution time, not processed frames.

@hansent hansent removed the claude-review Use to trigger AI review manually label Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants