feat: add SSL+HC fused classifier backend (ssl_fusion) - #91
feat: add SSL+HC fused classifier backend (ssl_fusion)#91PouyaMohseni wants to merge 28 commits into
Conversation
…ets by SSL compatibility
…eddings or source images
…=3, full-corpus tuned
…scale The DINO SimCLR checkpoint was trained on real colour manuscript photographs, but the ingest pipeline converted pages to greyscale before slicing the "real-pixel" crop, then faked RGB by channel replication at extraction time -- a genuine train/inference domain mismatch. Store and slice colour crops instead, and regenerate the Hufnagel/Square2 preset embeddings to match.
…t=4.0 Re-validated on the corrected colour-crop pipeline across leave-one-manuscript-out (Hufnagel/MS234/Antiphonal+NZ) and a single-manuscript stress test: the previous RBF kernel collapses onto majority classes whenever trained on one small/imbalanced manuscript (the common case -- a user selecting a single training preset). This linear config beat every alternative tested, including being the first to beat the handcrafted-feature kNN baseline on the single-manuscript stress test (79.4% vs 77.0%).
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds optional SSL-fusion classification with cached glyph features, companion embedding ingestion/export, API contracts, and frontend controls. Session classification remains backend-selectable, while exports remain read-only and sessions stay editable after completion. ChangesSSL fusion workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant UploadView
participant API
participant Session
participant SSLFusionClassifier
participant ViTExtractor
UploadView->>API: upload training XML and companion assets
API->>Session: create session with enriched glyphs
UploadView->>API: classify with ssl_fusion
API->>Session: select classification backend
Session->>SSLFusionClassifier: fit and predict glyphs
SSLFusionClassifier->>ViTExtractor: extract missing SSL features
ViTExtractor-->>SSLFusionClassifier: return embeddings
SSLFusionClassifier-->>Session: return classifications
Session-->>API: return updated session
API-->>UploadView: return classification result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/EditPanel.tsx (1)
103-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not make successful label updates depend on reclassification succeeding.
The edit mutations can persist changes before the selected classifier runs. If classification fails, cache refresh and related bookkeeping are skipped, leaving the UI stale despite successful server updates.
frontend/src/components/EditPanel.tsx#L103-L108: record the edit and refresh/retain undo state independently of classification success.frontend/src/hooks/useUpdateGlyphs.ts#L76-L80: invalidate the session after per-glyph updates even when classification rejects.frontend/src/hooks/useUpdateGlyphs.ts#L107-L111: invalidate the session after bulk updates even when classification rejects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/EditPanel.tsx` around lines 103 - 108, Make label persistence and session invalidation independent of classifier success: in frontend/src/components/EditPanel.tsx lines 103-108, ensure the edit and undo-state bookkeeping are retained and the session cache is refreshed even when classify.mutateAsync rejects; in frontend/src/hooks/useUpdateGlyphs.ts lines 76-80 and 107-111, ensure both per-glyph and bulk update flows invalidate the session after updates regardless of classification failure.
🧹 Nitpick comments (8)
core/data/presets/Square2.ssl_embeddings.npz (1)
1-1: 🧹 Nitpick | 🔵 TrivialBinary asset — verify regeneration/versioning strategy.
This is a generated binary artifact (precomputed SSL embeddings). Since it's derived data tied to a specific SSL extractor/model version, consider documenting how it's regenerated (e.g., via
generate_square2_ssl_embeddings.py) and whether large binary blobs like this should be tracked via Git LFS instead of committed directly to the repo, to avoid repo bloat as embeddings get regenerated over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/presets/Square2.ssl_embeddings.npz` at line 1, Document the regeneration and versioning strategy for the generated Square2 SSL embeddings, referencing generate_square2_ssl_embeddings.py and the extractor/model version used. Clarify whether this binary should remain in Git or be managed through Git LFS, and update repository tracking accordingly if the project’s policy requires it.frontend/src/components/UploadView.tsx (1)
168-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared training-args computation from
handleSubmit/handleAutoExport.
training/embeddings/images/presetNamesare recomputed identically across 4 call sites (2 inhandleSubmit, 2 inhandleAutoExport). A small helper (oruseMemo) would remove the duplication and keep the two flows in sync as new training-data fields get added later.♻️ Suggested extraction
+ function buildTrainingArgs() { + return { + trainingFiles: trainingFiles.length > 0 ? trainingFiles : undefined, + trainingEmbeddings: + trainingEmbeddings.length > 0 ? trainingEmbeddings : undefined, + trainingImages: trainingImages.length > 0 ? trainingImages : undefined, + trainingPresets: + selectedPresets.length > 0 ? selectedPresets : undefined, + }; + }Then spread
...buildTrainingArgs()into eachmutate(...)call.Also applies to: 204-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/UploadView.tsx` around lines 168 - 198, Extract the shared training-data argument construction from handleSubmit and handleAutoExport into a reusable helper, such as buildTrainingArgs, using trainingFiles, trainingEmbeddings, trainingImages, and selectedPresets with their existing empty-to-undefined behavior. Spread the helper result into all four createFromStaging.mutate and create.mutate calls, preserving the existing non-training fields and keeping both submission flows synchronized.core/ic_core/src/ic_core/classifier.py (1)
500-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
Protocolinstead ofobjectfor the classifier-factory contract.Widening the return type to
object(fromInteractiveClassifier) is a reasonable trade-off to support pluggable backends, but it means any caller that keeps the returned classifier to reuse.fit()/.predict_many()loses static-type checking entirely — the docstring even states the contract ("a zero-argument callable returning an unfitted classifier with.fit(),.predict_many()") but nothing enforces it for a type checker. A smallProtocolcaptures this structurally without requiringInteractiveClassifier/SSLFusionClassifierto share a common base class.♻️ Suggested Protocol
-from typing import Callable, Iterable, Sequence +from typing import Callable, Iterable, Protocol, Sequence + + +class TrainableClassifier(Protocol): + def fit(self, training_glyphs: Sequence[Glyph]) -> "TrainableClassifier": ... + def predict_many(self, glyphs: Sequence[Glyph]) -> list[Prediction]: ...def run_correction_stage( glyphs: Sequence[Glyph], training_glyphs: Sequence[Glyph] | None = None, *, k: int = DEFAULT_K, - classifier_factory: Callable[[], object] | None = None, -) -> tuple[list[Glyph], object]: + classifier_factory: Callable[[], TrainableClassifier] | None = None, +) -> tuple[list[Glyph], TrainableClassifier]:Also applies to: 536-550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/ic_core/src/ic_core/classifier.py` around lines 500 - 501, Define a classifier protocol exposing the required unfitted-classifier methods, including fit() and predict_many(), and use it for the classifier_factory return type and the enclosing function’s returned classifier type instead of object. Ensure InteractiveClassifier and SSLFusionClassifier satisfy the protocol structurally so callers retaining the classifier preserve static type checking.core/tests/test_ssl_classifier.py (1)
173-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAsserting on the private
_calibratedflag couples the tests to internals.Consider exposing a small read-only property (e.g.
is_calibrated) onSSLFusionClassifierand asserting on that instead; behaviourally the confidence assertions already carry most of the signal.Also applies to: 199-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tests/test_ssl_classifier.py` around lines 173 - 176, Expose a read-only public calibration-status property such as is_calibrated on SSLFusionClassifier, backed by the existing calibration state, and update the affected tests to assert through that property instead of accessing the private _calibrated flag. Keep the existing confidence and class-name assertions unchanged.core/ic_core/src/ic_core/ssl_classifier.py (1)
157-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMixed precomputed/live batches assume identical dimensionality.
X_sslis allocated fromlen(precomputed[0].ssl_embedding)and then assigned the live extractor output; if the configured checkpoint'scls_meanwidth differs from the shipped preset embeddings (e.g. a different backbone), this fails with an opaque numpy broadcast error rather than a message pointing at the checkpoint/preset mismatch. A short explicit width check would be worth it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/ic_core/src/ic_core/ssl_classifier.py` around lines 157 - 166, In the mixed precomputed/live branch, validate that the live output from _ssl_features has the same embedding width as the precomputed ssl_embedding width before assigning into X_ssl. Raise a clear error identifying the checkpoint-versus-preset dimensionality mismatch instead of allowing a NumPy broadcast failure.api/src/ic_api/main.py (1)
1511-1517: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull model pass runs while holding the per-session lock.
extract_ssl_embeddingscan load a ViT and run a batched forward pass over every selected glyph; doing it insidewith store.session(...)serialises every other request for that session behind it (classify, glyph updates) for potentially minutes. Snapshotting the selected glyphs under the lock and extracting outside it would keep the session responsive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/ic_api/main.py` around lines 1511 - 1517, Update the flow around the store.session block in the export handler so _select_export_glyphs snapshots the required glyph data while the per-session lock is held, then exit the context before calling extract_ssl_embeddings. Preserve the existing selected-glyph inputs and output serialization, but ensure the model load and batched inference cannot hold the session lock.core/scripts/generate_hufnagel_ssl_embeddings.py (2)
57-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
match_glyphs_to_source_pagesinstead of the hand-rolled RLE/matching duplicate.
_decode_rle_mask,_parse_preset_glyphs(regex over XML) and the per-glyph brute-force loop reimplement whatic_core.io_xml.load_glyphs+ic_core.ssl_preset_embeddings.match_glyphs_to_source_pagesalready do — exactly the shape the Square2 script uses. Keeping two matching implementations risks the two presets' embeddings drifting apart (threshold, greyscale conversion, glyph ordering).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/scripts/generate_hufnagel_ssl_embeddings.py` around lines 57 - 99, Replace the local _decode_rle_mask, _parse_preset_glyphs, and per-glyph page-matching loop in main with ic_core.io_xml.load_glyphs and ic_core.ssl_preset_embeddings.match_glyphs_to_source_pages, following the existing Square2 script usage. Preserve the resulting glyph ordering, matched crops, and unmatched handling while using the shared implementation for thresholding and grayscale conversion; remove the now-unused duplicate helpers and related logic.
42-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHard-coded absolute paths from a developer's machine.
SOURCE_PAGESpoints at/Users/home/Desktop/DDMAL/..., so this script only runs on one laptop even though the siblingcore/scripts/generate_square2_ssl_embeddings.pyresolves its page relative to the repo (and the pages appear to live undercore/data/train/). Suggest repo-relative paths with an env-var override.♻️ Proposed change
-SOURCE_PAGES = [ - Path( - "/Users/home/Desktop/DDMAL/standalone-interactive-classifier/core/data/train/hufnagel_example_826dd1b4.png" - ), - Path( - "/Users/home/Desktop/DDMAL/standalone-interactive-classifier/core/data/train/hufnagel_example_a77ec16f.png" - ), - Path( - "/Users/home/Desktop/DDMAL/standalone-interactive-classifier/core/data/train/hufnagel_example_fbed8126.png" - ), -] +_TRAIN_DIR = Path(__file__).resolve().parents[1] / "data" / "train" +SOURCE_PAGES = [ + _TRAIN_DIR / "hufnagel_example_826dd1b4.png", + _TRAIN_DIR / "hufnagel_example_a77ec16f.png", + _TRAIN_DIR / "hufnagel_example_fbed8126.png", +]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/scripts/generate_hufnagel_ssl_embeddings.py` around lines 42 - 52, Update SOURCE_PAGES in generate_hufnagel_ssl_embeddings.py to resolve the training page paths relative to the repository or script location instead of using developer-specific absolute paths, while supporting an environment-variable override for the data root. Preserve the existing three page filenames and align the resolution approach with generate_square2_ssl_embeddings.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/ic_core/src/ic_core/ssl_classifier.py`:
- Around line 265-293: The predict_many method must handle glyphs lacking both
ssl_embedding and image_gray_b64 before calling _fused_features, matching
fit()’s filtering behavior. Filter or otherwise validate these crop-less glyphs
up front so one invalid glyph cannot cause an unhandled crop_glyph_to_square
failure; if rejecting them, raise an actionable error identifying the affected
glyphs.
In `@core/ic_core/src/ic_core/ssl_extractor.py`:
- Around line 161-162: Update the checkpoint loading call in the SSL extractor
to pass weights_only=True to torch.load while preserving the existing CPU map
location and subsequent backbone.load_state_dict(state) flow.
- Around line 298-337: Update _apply_adapters and _apply_ssf to traverse
ViTModel’s encoder.layer modules instead of backbone.layers, targeting
attention.output.dense, intermediate.dense, and output.dense. Preserve the
existing adapter hook behavior and SSF wrapping while ensuring method.json
checkpoint loading can inject modules before load_state_dict.
In `@core/ic_core/src/ic_core/ssl_preset_embeddings.py`:
- Around line 85-93: Update the glyph/embedding validation before the list
comprehension in the preset embedding loader to require a 2-D floating embedding
matrix with finite values, raising ValueError for invalid shape, dtype, or
non-finite entries. Preserve the existing count-mismatch validation and only
attach embeddings after all validation succeeds.
In `@core/ic_core/src/ic_core/state.py`:
- Around line 281-288: Update the `backend` parameter docstring to describe
`ssl_fusion` as using the SSL+HC fused linear-kernel SVM classifier, calibrated
via `CalibratedClassifierCV`, instead of calling it a logistic-regression
classifier; preserve the existing requirements and references.
In `@core/tests/test_ssl_classifier.py`:
- Around line 169-171: Update the classifier factories in the affected tests,
including test_fits_with_few_examples_per_class_shrinks_cv, to construct
SSLFusionClassifier with _SSL_CHECKPOINT instead of leaving checkpoint unset.
Ensure both tests use a factory that forwards this checkpoint so they bypass
network downloads while preserving their existing test behavior.
In `@frontend/src/constants/classifierBackends.ts`:
- Around line 16-24: Update the SSL-fusion classifier entry’s user-facing label
and title to identify linear-kernel SVM rather than logistic regression, and
revise the requirements text to state that uploaded GameraXML files are
supported when companion embeddings or source images are available. Keep the
remaining SSL checkpoint, preset, and server-extra requirements accurate.
In `@frontend/src/hooks/useCreateSession.ts`:
- Around line 25-36: Handle rejection from the fire-and-forget classify call in
reclassifyIfSslFusionChosen by adding explicit failure handling that surfaces
the error instead of allowing an unhandled promise rejection. Keep successful
query invalidation unchanged.
---
Outside diff comments:
In `@frontend/src/components/EditPanel.tsx`:
- Around line 103-108: Make label persistence and session invalidation
independent of classifier success: in frontend/src/components/EditPanel.tsx
lines 103-108, ensure the edit and undo-state bookkeeping are retained and the
session cache is refreshed even when classify.mutateAsync rejects; in
frontend/src/hooks/useUpdateGlyphs.ts lines 76-80 and 107-111, ensure both
per-glyph and bulk update flows invalidate the session after updates regardless
of classification failure.
---
Nitpick comments:
In `@api/src/ic_api/main.py`:
- Around line 1511-1517: Update the flow around the store.session block in the
export handler so _select_export_glyphs snapshots the required glyph data while
the per-session lock is held, then exit the context before calling
extract_ssl_embeddings. Preserve the existing selected-glyph inputs and output
serialization, but ensure the model load and batched inference cannot hold the
session lock.
In `@core/data/presets/Square2.ssl_embeddings.npz`:
- Line 1: Document the regeneration and versioning strategy for the generated
Square2 SSL embeddings, referencing generate_square2_ssl_embeddings.py and the
extractor/model version used. Clarify whether this binary should remain in Git
or be managed through Git LFS, and update repository tracking accordingly if the
project’s policy requires it.
In `@core/ic_core/src/ic_core/classifier.py`:
- Around line 500-501: Define a classifier protocol exposing the required
unfitted-classifier methods, including fit() and predict_many(), and use it for
the classifier_factory return type and the enclosing function’s returned
classifier type instead of object. Ensure InteractiveClassifier and
SSLFusionClassifier satisfy the protocol structurally so callers retaining the
classifier preserve static type checking.
In `@core/ic_core/src/ic_core/ssl_classifier.py`:
- Around line 157-166: In the mixed precomputed/live branch, validate that the
live output from _ssl_features has the same embedding width as the precomputed
ssl_embedding width before assigning into X_ssl. Raise a clear error identifying
the checkpoint-versus-preset dimensionality mismatch instead of allowing a NumPy
broadcast failure.
In `@core/scripts/generate_hufnagel_ssl_embeddings.py`:
- Around line 57-99: Replace the local _decode_rle_mask, _parse_preset_glyphs,
and per-glyph page-matching loop in main with ic_core.io_xml.load_glyphs and
ic_core.ssl_preset_embeddings.match_glyphs_to_source_pages, following the
existing Square2 script usage. Preserve the resulting glyph ordering, matched
crops, and unmatched handling while using the shared implementation for
thresholding and grayscale conversion; remove the now-unused duplicate helpers
and related logic.
- Around line 42-52: Update SOURCE_PAGES in generate_hufnagel_ssl_embeddings.py
to resolve the training page paths relative to the repository or script location
instead of using developer-specific absolute paths, while supporting an
environment-variable override for the data root. Preserve the existing three
page filenames and align the resolution approach with
generate_square2_ssl_embeddings.py.
In `@core/tests/test_ssl_classifier.py`:
- Around line 173-176: Expose a read-only public calibration-status property
such as is_calibrated on SSLFusionClassifier, backed by the existing calibration
state, and update the affected tests to assert through that property instead of
accessing the private _calibrated flag. Keep the existing confidence and
class-name assertions unchanged.
In `@frontend/src/components/UploadView.tsx`:
- Around line 168-198: Extract the shared training-data argument construction
from handleSubmit and handleAutoExport into a reusable helper, such as
buildTrainingArgs, using trainingFiles, trainingEmbeddings, trainingImages, and
selectedPresets with their existing empty-to-undefined behavior. Spread the
helper result into all four createFromStaging.mutate and create.mutate calls,
preserving the existing non-training fields and keeping both submission flows
synchronized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 479fda2e-9e10-4e07-815a-e21502b04f4b
⛔ Files ignored due to path filters (3)
api/uv.lockis excluded by!**/*.lockcore/data/train/Einsiedeln__Stiftsbibliothek__Codex_611_014r.jpgis excluded by!**/*.jpgcore/ic_core/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
api/pyproject.tomlapi/src/ic_api/main.pyapi/src/ic_api/schemas.pyapi/tests/test_api.pycore/data/presets/Hufnagel.ssl_embeddings.npzcore/data/presets/Square2.ssl_embeddings.npzcore/ic_core/pyproject.tomlcore/ic_core/src/ic_core/classifier.pycore/ic_core/src/ic_core/glyph.pycore/ic_core/src/ic_core/image.pycore/ic_core/src/ic_core/ingest.pycore/ic_core/src/ic_core/ssl_classifier.pycore/ic_core/src/ic_core/ssl_extractor.pycore/ic_core/src/ic_core/ssl_preset_embeddings.pycore/ic_core/src/ic_core/state.pycore/scripts/generate_hufnagel_ssl_embeddings.pycore/scripts/generate_square2_ssl_embeddings.pycore/tests/test_ssl_classifier.pycore/tests/test_ssl_preset_embeddings.pyfrontend/src/api/sessions.tsfrontend/src/components/EditPanel.tsxfrontend/src/components/SplitDialog.tsxfrontend/src/components/Toolbar.tsxfrontend/src/components/UploadView.tsxfrontend/src/constants/classifierBackends.tsfrontend/src/hooks/useAutoExport.tsfrontend/src/hooks/useClassify.tsfrontend/src/hooks/useCreateSession.tsfrontend/src/hooks/useExportEmbeddings.tsfrontend/src/hooks/useUpdateGlyphs.tsfrontend/src/store/uiStore.tsfrontend/src/types/api.ts
👮 Files not reviewed due to content moderation or server errors (1)
- core/data/presets/Hufnagel.ssl_embeddings.npz
…cs, checkpoint security)
…l_fusion UI label
…ribute-path near-miss
|
@coderabbitai, review |
|
✅ Action performedReview finished.
|
|
By examine the result manually, I found a few UI issue and have a few comments about the performance. UI issues:
Comments:
Please view the changes of this PR in the stand-alone version and the mothra embedded version separately to see the compatibility. Merge to |
Summary
Adds an optional
ssl_fusionclassifier backend — DINO SimCLR embeddings fused with the existing 29-dim handcrafted (HC) features — as an alternative to the default kNN classifier, selectable per session.Changes
SSLFusionClassifierfuses standardized SSL + HC feature blocks via weighted concatenation, classifies with a linear-kernel SVM.Glyph.image_gray_b64the SSL extractor needs real colour pixels (matching what the DINO checkpoint was trained on), not the binary mask kNN uses. Colour was previously discarded at ingest and faked back via channel replication — a real train/inference mismatch, now fixed.Session.classifyand the/classifyAPI endpoint (backend: "knn" | "ssl_fusion"); new Model selector in the frontend toolbar and upload screen..ssl_embeddings.npzsupport for built-in presets (Hufnagel, Square2) and for uploaded GameraXML training files, sossl_fusiondoesn't require a live model pass on every session.match_glyphs_to_source_pagesrecovers real crops from bbox+mask when only a GameraXML file (no stored crop) is uploaded./export-embeddingsendpoint + UI button, so a session's SSL features can be exported and re-imported as a future preset.Summary by CodeRabbit
New Features
Bug Fixes