refactor(timm_mobilenetv3): migrate to Task SDK - #1308
Conversation
Implement the image class-score Task with complete owned logits and family-owned metadata. Preserve preprocessing and the original reference oracle, and exercise direct C and C++ consumers in the existing single-device E2E. Signed-off-by: yifeif-nv <277870278+yifeif-nv@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummaryMigrates The runtime returns all checkpoint logits in class order. It publishes class count, vocabulary ID, and labels as family-owned metadata. It validates input, output, and runtime configuration. The manifest, builder, support declarations, documentation, benchmark reference, and tests now use the semantic task. Direct C11 and C++17 SDK consumers extend single-device E2E coverage. The graph, preprocessing, JPEG CLI path, and top-1 oracle remain unchanged. Validation included CPU tests, non-GPU/non-TRT/non-E2E tests, one single-device E2E case, source-quality checks, benchmark-reference tests, and Architecture impact
Review outcomeHUMAN REVIEW REQUIRED No current review findings were supplied, so severity counts are unavailable. Human review remains required because compatibility and blast-radius questions remain unresolved. WalkthroughThe MobileNetV3 family now exposes ChangesMobileNetV3 image-to-class-scores
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant SDKConsumer
participant TimmMobilenetv3ImageClassificationPipeline
participant ITrtModule
SDKConsumer->>TimmMobilenetv3ImageClassificationPipeline: submit image_to_class_scores request
TimmMobilenetv3ImageClassificationPipeline->>ITrtModule: preprocess input and run inference
ITrtModule-->>TimmMobilenetv3ImageClassificationPipeline: return float32 logits
TimmMobilenetv3ImageClassificationPipeline-->>SDKConsumer: return LabelScoresResult with logits and metadata
Merge Risk: ⚪ Minimal · up to The new benchmark executes and validates its family script independently of the inert backend label, so no current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 12 files. (2 skipped: 2 unsupported.) Full details: Benchmark Validation IntegrityExplanation The benchmark adds asymmetric output validation. The native candidate times Resolution Align output validation at the measurement boundary. Either move candidate-only output checks outside the timed task call, or add equivalent shape, dtype, class-count, and validity checks to the reference timed invocation before it returns. Keep reduction, synchronization, host transfer, and serialization placement identical on both paths, then retain tests that verify the event ordering. Comment |
Use the existing family-owned reference protocol for the migrated class-score Task. Time inference and complete host scores, then perform classification and reporting outside the timer. Preserve the existing workload, timing declarations and comparison criteria; the pre-existing preprocessing scope difference remains documented. Signed-off-by: yifeif-nv <277870278+yifeif-nv@users.noreply.github.com>
|
Review follow-up for The family-owned reference now places argmax and reporting after timing, while The remaining warning is narrower than a failed inference/parity result:
The existing preprocessing timing-scope difference remains documented in the |
Background
Migrate timm MobileNetV3 to the semantic Task SDK introduced by #1226.
Every changed file is under
families/timm_mobilenetv3/; the shared C ABI,C++ wrapper, CLI, benchmark and CI discovery already support this Task.
Exit Criteria
image_to_class_scores, returning everyoriginal logit in checkpoint class order through the C and C++ SDKs.
pass before merge; local results are not a remote Pre-merge claim.
Implementation
IModelandIImageToClassScores; the family returns its ownbindrecord with no optional runtime Config fields.image_to_class_scores.Return complete owned logits, not a top-k subset or softmax probabilities.
family's runtime metadata. Missing identity stays empty; no hash, invented
name or local checkpoint path is used as a cross-model identity.
known/unknown class identity and rejection of unsupported Config.
Both read the result after releasing the model and must agree on every score
for identical RGB pixels. The original JPEG CLI path and timm top-1 check
remain unchanged; different JPEG decoders are not claimed bitwise equivalent.
dependency. No shared registry, workflow or new environment variable is added.
Follow-up: family-owned benchmark timing
The migration moved native argmax outside the timed Task call, while the old
reference still timed argmax and summary generation. The owning family now
provides
tests/performance.yamlandtests/performance_reference.pythroughthe existing reference-script mechanism. Model inference, full host float32
scores and synchronization are timed; classification and reporting are not.
The original entry ID, workload, FP16 policy, 3 warmups, 10 measurements, 5%
margin and top-class oracle are preserved. The existing reference policy still
excludes input preparation whereas the native Task includes family preprocessing;
this change does not claim to remove that pre-existing scope difference or
establish a performance improvement. No shared file changes.
Change categories
Public C layouts and bundle format 1 do not change. The family's primary Task
and runtime metadata do change: rebuild old classification bundles. There is
no old Task alias, compatibility adapter or fallback execution path.
Validation
Commands and Results
The following original migration results belong to
8d317e3e86cef6dda577694ca15aa082610b900f, using the repository's nativedependencies and existing E2E environment. Follow-up validation is listed
separately below; these results are not new-head CI results.
cmake --build build --target trtmc trtmc_backend_trt test_timm_mobilenetv3_task_contract test_timm_mobilenetv3_image_preprocess:passed, including the public C/C++ consumer binaries.
ctest --test-dir build --output-on-failure -R '^timm_mobilenetv3_(task_contract|image_preprocess)$':passed, 2/2 CPU tests.
python -m pytest families/timm_mobilenetv3/tests -m 'not gpu and not trt and not e2e' -q:17 passed; the explicitly unselected real E2E case was skipped.
python -m pytest families/timm_mobilenetv3/tests/test_e2e.py --e2e-testcase mobilenetv3-large-100-ra-in1k -q:1 real single-device case passed. CLI/C/C++ retain the original top-1 oracle;
the two SDK consumers agree on all 1000 logits and class metadata.
python3 -m tools.community_ci source-quality --base ed98d6affe9e61be98ead407c1fd77e4155d6b75:passed legal headers, inventory, complexity, formatting and 212 contract tests.
git diff --check: passed. Existing benchmark resolution selects the newTask and retains its
classifyoperation without a central suite edit.Additional checks for the timing follow-up:
python3 -m pytest families/timm_mobilenetv3/tests/test_performance_reference.py -q:15 passed, including timer ordering, receipt identity and invalid-output checks.
explicitly unselected real E2E case skipped.
workload on the pinned checkpoint below: completed, 10 timing samples,
1000 finite scores and top-class 656. This is reference execution evidence,
not a new paired speedup or full-migration E2E claim.
Hardware, Environment, and Revisions
ed98d6affe9e61be98ead407c1fd77e4155d6b75(feat(api): add semantic Task C ABI and C++ SDK #1226).8d317e3e86cef6dda577694ca15aa082610b900f.bd007e2fe3fb859c4244b5efc0fc1f3c6d89a585.Production runtime, builder and original E2E sources are unchanged by this follow-up.
PyTorch 2.12.0+cu130 and timm 1.0.28; FP16 as in the original manifest.
timm/mobilenetv3_large_100.ra_in1krevision96f46a1c52932f27492dff66c72378eb99b443a7; original checked-in test image.Not Run / Remaining Gaps
results do not qualify this update.
Contributor Self-Review
Reviewed the family-only diff, unchanged preprocessing/reference, complete
outputs and public evidence, with independent technical and publication review.
Notes For Future Readers
#1226 is already merged. This PR is independent of the PatchTSMixer migration
and other families.
families/timm_mobilenetv3/README.mddescribes the new Task,bundle rebuild and direct SDK validation commands. Runtime and family DSOs
upgrade together; obsolete shared execution interfaces are not removed here.
Risk level
This changes one family's public Task identity and bundle metadata. The original
model/CLI oracle and additional complete SDK-output checks constrain regression.