Repository navigation
fix: keep float32 output for mean pooling models - #753
jaysheeldodia wants to merge 3 commits into
Conversation
|
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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughMean-pooling post-processing now casts embeddings back to the model output dtype after pooling and normalization. The custom embedding path casts only floating-point outputs. Tests check float32 and float16 dtype preservation and verify normalized values for float16 inputs. Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change preserves model output types without introducing an identified numerical regression. It is mergeable subject to normal test checks. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The inspected changes restore floating-point output precision after pooling without adding access, privileges, or external side effects. No material security risk was identified in these changes. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hey @jaysheeldodia Unfortunately, I didn't see the PR and implemented the fix on my own in #752 |
|
Hey @joein |
Fixes #752
Models that use mean pooling have been returning
float64embeddings, even though the ONNX model outputsfloat32. The values are correct, but the embeddings take twice the memory.tobytes()also writes the wrong byte size, and anything expectingfloat32rejects them.Why it happened
mean_poolingmultiplies thefloat32embeddings by theint64attention mask, and NumPy turns the result intofloat64. A final.astype(np.float32)used to hide this, but #492 removed it. Custom mean-pooling models from #479 had the same problem.The fix
The result is now converted back to the model's output format in
PooledEmbedding,PooledNormalizedEmbeddingandCustomTextEmbedding. For custom models this only happens for float outputs, so integer outputs aren't cut off. The conversion comes afternormalize(), because converting afloat16result first can overflow the norm and turn the embedding into all zeros.The values don't change. The new output is exactly the old output in
float32, with no speed difference and half the memory.Tests
test_embeddingandtest_mock_add_custom_modelsnow also check the output format.test_mean_pooling_keeps_model_dtypechecks the format and the values for both built-in mean-pooling classes, withfloat32andfloat16input.test_custom_mean_pooling_keeps_float16_without_overflowchecks that afloat16custom model with large values still gets the right result.All Submissions:
New Feature Submissions:
pre-commitwithpip3 install pre-commitand set up hooks withpre-commit install?