training: versioned TrainedModels per template - #125
Merged
Conversation
Each retrain now produces a new TrainedModel version instead of overwriting the previous artifact. v1 keeps the bare TemplateModel.model_id for backwards compatibility; v2+ append "-v<n>" so existing benchmarks and documents that reference older Azure model names keep resolving. Schema changes: - TrainedModel: add version, is_active, deleted_at, dataset_snapshot. Drop unique(template_model_id); add unique(template_model_id, version) + index(template_model_id, is_active). Existing rows backfilled to version=1, is_active=true. - TrainingJob: add target_model_id and target_version so the poller and consumers don't have to re-derive the version at completion time. Service changes: - TrainingService.startTraining: never delete tracked TrainedModel rows on retrain; mint a versioned Azure model id; persist target on the job. - TrainingPollerService: capture a labeled-document snapshot at success, demote any prior active version, create the new version with is_active + dataset_snapshot. - TrainingService gains listTrainedVersions, getTrainedVersionSnapshot, setActiveTrainedVersion, deleteTrainedVersion. Deletion blocks on the active version and on any benchmark definition that pins the model id; successful deletes tombstone the row (deleted_at) and remove the Azure artifact best-effort. - TrainingDbService gains getNextVersionNumber, demoteActiveTrainedModels, setActiveTrainedModel, tombstoneTrainedModel, buildTrainedModelSnapshot, and filters tombstoned rows out of findAllTrainedModelIds (OCR picker). Controller exposes new routes under /api/template-models/:modelId/training/ versions[/:versionId][/snapshot|activate]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a fifth tab to ModelDetailPage that lists every trained version of a template (including tombstoned), shows the active version, and exposes the manual-management actions matching the new backend: - View training data: opens a drawer with the labeled documents and their labels as captured at training time. - Set active: promote a non-deleted version to be the one resolved at OCR / benchmark time. - Delete: tombstones the version and deletes the Azure model. Disabled when the version is active or referenced by a benchmark definition (the backend enforces and surfaces a message we render in a notification). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Versions tab auto-refreshes while training is in progress, and forces a final refetch when the active job transitions to idle so the new version appears without a manual page reload. - Header "Copy model ID" now copies the active trained version's model id (e.g. km-invoice-v3) and shows a small "v3" indicator next to the code. Falls back to the bare template id when no active version exists. - TrainingPanel relabels "Azure Model ID" to "Template Model ID" and adds a hint that retrains use a versioned suffix; removes the redundant copy button (use the header). - Snapshot drawer is now an Accordion of files; labels are collapsed by default. Each row shows filename + label-count badge so the file list is scannable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /template-models cards still showed the bare TemplateModel.model_id even after retrains had moved the active Azure model name to a versioned id. Backend: include the active TrainedModel slice (one row per template) on findAllTemplateModels and findTemplateModel, and reshape it onto a single active_trained_model field on TemplateModelData. Avoids N+1 lookups from the list page. Frontend: - ModelCard now renders + copies active_trained_model.model_id when present, with a small "v3" indicator. - ModelDetailPage drops its extra useTrainedVersions call and reads the active version from the template-model query directly. - TrainedVersionsPanel + useTrainedVersions invalidate the template-model queries on activate/delete and on training-finished transitions, so the list and detail headers update without a page refresh. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alex-struk
marked this pull request as ready for review
May 7, 2026 23:08
alex-struk
requested review from
NoorChasib,
antsand,
dbarkowsky and
kmandryk
as code owners
May 7, 2026 23:08
dbarkowsky
approved these changes
May 12, 2026
dbarkowsky
left a comment
Collaborator
There was a problem hiding this comment.
Nothing breaking, but some comments worth capturing in another ticket if not addressed before merge.
apiService swallows axios errors into { success: false, message, data }.
activateMutation was returning response.data unconditionally, so backend
rejections (e.g. ConflictException when activating a deleted version)
were silently treated as success and the user saw no error.
Routes both activate and delete responses through a small unwrap helper
that throws on !success, removing the inconsistency between them.
Addresses PR #125 review comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
getTrainedVersionSnapshot, setActiveTrainedVersion, and deleteTrainedVersion
were loading every version for the template and filtering by id in JS. That
incidentally enforced the templateModelId/trainedModelId scoping; switching
to a naive findUnique({ id }) would have silently dropped that check.
Add findTrainedModelForTemplate(templateModelId, trainedModelId, opts) to
TrainingDbService and use it in all three callers — single-row query, with
the scope check now explicit in the where clause.
Addresses PR #125 review comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After a successful Azure training run, the poller demoted the prior active TrainedModel and then created the new one in two separate writes. A crash between them would leave the template with zero active versions, and downstream consumers (findActiveTrainedModel, the OCR picker, the active trained-model on the template detail page) would silently see nothing. Add TrainingDbService.replaceActiveTrainedModel(templateModelId, data) that runs both writes inside prisma.$transaction, and switch the poller to it. Addresses PR #125 review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
getNextVersionNumber + the eventual createTrainedModel were not atomic, so two concurrent training starts on the same template would compute the same next version, both upload to Azure under the same versioned model id, and the loser's poller would later hit the @@unique(template_model_id, version) constraint. By then the Azure model would already exist and the job row would be left in a partial state. Rather than serialize version assignment (which still permits the double Azure run), refuse the second start outright: findInFlightJobForTemplate returns any non-terminal job for the template, and startTraining throws ConflictException if one exists. The user-visible behaviour now matches the obvious intent — wait for or cancel the current training. Addresses PR #125 review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Thanks for the review @dbarkowsky — addressed all five in separate commits on this branch:
Backend suite: 2075 passing (+8 new tests covering |
5cd7c9a dropped the forwardRef on TrainingModule's BenchmarkModule import on the premise that no cycle existed. That premise was wrong — the cycle runs through OcrModule: TrainingModule → BenchmarkModule → OcrModule → TrainingModule OcrModule imports TrainingModule because the OCR controller depends on TrainingService for trained-model lookup. Without forwardRef, BenchmarkModule is referenced at file-eval time before its export has initialized, and Nest crashes at boot with "Cannot access 'BenchmarkModule' before initialization". Restores both the module-level forwardRef and the constructor-level @Inject(forwardRef(() => BenchmarkDefinitionDbService)), and adds a comment documenting the cycle so a future reader doesn't make the same mistake. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…DbService Points future readers at the module-level comment in training.module.ts that explains the TrainingModule → BenchmarkModule → OcrModule cycle, so the @Inject(forwardRef(...)) isn't mistaken for cargo-cult. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Each retrain previously overwrote the prior
TrainedModeland Azure model. Benchmarks pinnedctx.modelId.defaultValue: "<model_id>"could no longer tell what they had measured against, and there was no way to look back at "what was v2 trained on?".This branch makes
TrainedModelversioned per template, with a manual-management UI:TemplateModel.model_idfor backwards compatibility — existing benchmarks/documents that reference it keep resolving.<base>-v<n>. The new version becomes active automatically; the prior one is demoted but kept.km-invoice-v3) instead of the bare stub. Updates without a manual refresh after retrain / activate / delete.Schema
Two migrations under
apps/shared/prisma/migrations/:20260501225325_add_trained_model_versioning— addsversion,is_active,deleted_at,dataset_snapshotontrained_models; replaces the unique ontemplate_model_idwith(template_model_id, version)plus a(template_model_id, is_active)index. Existing rows are backfilled toversion = 1,is_active = true.20260501225925_training_job_target_version— addstarget_model_idandtarget_versionontraining_jobsso the poller knows the versioned Azure name without re-deriving it.No data destruction; existing single-version templates upgrade transparently.
Test plan
npm run db:migratefromapps/backend-servicesapplies cleanly (already applied locally).km-invoice); existing benchmarks/documents continue resolving.km-invoice-v2), the prior version is demoted, the new one becomes active, and the Versions tab shows both./api/template-models/...trained-model-ids) excludes tombstoned versions.Notes
ModelVersionrow, but that's out of scope here and would be a separate migration.🤖 Generated with Claude Code