Skip to content

minor: add missing dimension validation to predict_batch()#3

Merged
adrida merged 1 commit into
adrida:mainfrom
ggSohamgg:fix/predict-batch-dimension
Jun 8, 2026
Merged

minor: add missing dimension validation to predict_batch()#3
adrida merged 1 commit into
adrida:mainfrom
ggSohamgg:fix/predict-batch-dimension

Conversation

@ggSohamgg

Copy link
Copy Markdown
Contributor
fix: add dimension validation to predict_batch()

predict() validates embedding dim against manifest.embedding_dim and raises
a clean ValueError on mismatch. predict_batch() was missing the same check,
so sklearn's internal error bubbled up instead.

minor inconsistency, not a critical bug — just adding the identical guard
to predict_batch() for consistency + a test to cover it.

@adrida adrida left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this, clean catch. predict() had the guard and predict_batch() didn't, so a wrong-dim array hit route_pipeline and surfaced sklearn's internal error instead. The fix mirrors the existing check exactly: same placement after _to_embeddings, and manifest.embedding_dim is Optional so the is not None guard matches predict()'s legacy-artifact tolerance. Test covers it. Merging, thank you.

@adrida adrida merged commit c0a807f into adrida:main Jun 8, 2026
5 checks passed
@adrida adrida mentioned this pull request Jun 8, 2026
adrida added a commit that referenced this pull request Jun 8, 2026
Ships everything since 0.1.1 (0.1.2 was bumped in-tree but never tagged):
predict_batch() dim guard (#3), FitConfig/EmbeddingConfig fail-fast
validation (#4), update() fail-fast on missing all_traces.jsonl (#5),
plus the skip_candidates knob and fit() progress logging.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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