Skip to content

OpenConceptLab/ocl_issues#2388 | encoder_model param for reranking #839

Open
snyaggarwal wants to merge 4 commits intomasterfrom
issues#2388
Open

OpenConceptLab/ocl_issues#2388 | encoder_model param for reranking #839
snyaggarwal wants to merge 4 commits intomasterfrom
issues#2388

Conversation

@snyaggarwal
Copy link
Copy Markdown
Contributor

Linked Issue

Closes OpenConceptLab/ocl_issues#2388

@snyaggarwal snyaggarwal requested a review from paynejd April 6, 2026 05:59
Comment thread core/concepts/views.py Fixed
Copy link
Copy Markdown
Member

@paynejd paynejd left a comment

Choose a reason for hiding this comment

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

Review — OpenConceptLab/ocl_issues#2388

1. Narrow the exception handler (core/concepts/views.py)

The except Exception as e catches everything including SystemExit, OOM, and DB errors. Narrow to the specific failures that model loading/inference can raise:

except (ValueError, RuntimeError, OSError) as e:
    return Response({'detail': str(e)}, status=status.HTTP_400_BAD_REQUEST)

2. Change Closes to Ref in PR body

Both this PR and oclmap#8 say "Closes OpenConceptLab/ocl_issues#2388". Whichever merges first will auto-close the issue before the other ships. Suggest changing this PR body to "Ref OpenConceptLab/ocl_issues#2388" and letting oclmap#8 (the final piece) carry the Closes.

3. Consider API-level permission check for encoder_model

The issue specifies only core team users should access custom reranker config. The oclmap PR gates the UI on isCoreUser, but the $rerank/ endpoint accepts encoder_model from anyone — a non-core user could call the API directly with a custom model. Is this intentionally open at the API level? If not, add a permission check.

4. No input validation on encoder_model

An arbitrary string can be saved to the encoder_model field. The clean() method resets empty values to the default, but garbage strings persist in the DB and will cause errors on every subsequent rerank attempt. Consider a basic format check (e.g. must contain /, strip whitespace) or validate that the model can be loaded before saving.

5. Include the encoder model used in the rerank response

For traceability and debugging, include the model name in the rerank response so the frontend can log which model produced the scores for a given row:

return Response({'results': results, 'encoder_model': reranker.model_name or self.default_model})

This supports the frontend logging which model was used per row — important when users are experimenting with different reranker models.


Related follow-up tickets filed:

@snyaggarwal
Copy link
Copy Markdown
Contributor Author

snyaggarwal commented Apr 14, 2026

Addressed 1, 3 and 4.

Include the encoder model used in the rerank response

Mapper logs which model was used.

Comment thread core/concepts/views.py Dismissed
Copy link
Copy Markdown
Member

@paynejd paynejd left a comment

Choose a reason for hiding this comment

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

Follow-up Review

Items 1, 3, and 4 from the original review are properly addressed — nice work, Sunny. Two small fixes below, then I think this is good to merge.

PR body: ClosesRef

The body still says Closes OpenConceptLab/ocl_issues#2388 — since oclmap#8 also targets the same issue, whichever merges first will auto-close it. Suggest changing to Ref and letting the final piece carry the Closes.

CORE_USER_GROUP constant

The diff imports it from core.users.constants but doesn't show it being added — just confirming it already exists there?

Model name validation

Not a blocker, but worth noting: a typo'd model name (e.g. "not-a-real-model") will persist in the DB and cause 400s on every rerank call. A simple format check (must contain /) would catch obvious garbage. Fine to defer to a follow-up if preferred.

CodeQL alert on line 1032

The str(e) in the (ValueError, RuntimeError, OSError) handler is still flagged by GitHub Advanced Security. Probably fine for these exception types, but FYI if the team wants a clean scan.

Comment thread core/concepts/views.py Outdated
Copy link
Copy Markdown
Member

@paynejd paynejd left a comment

Choose a reason for hiding this comment

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

One more — bug in the generic exception handler.

Comment thread core/concepts/views.py Outdated
@snyaggarwal
Copy link
Copy Markdown
Contributor Author

CORE_USER_GROUP constant
The diff imports it from core.users.constants but doesn't show it being added — just confirming it already exists there?

CORE_USER_GROUP does exists in core.user.constants

Model name validation

Dont want to do strict / check, not sure if there might be model names without scopes like all-MiniLM-L6-v2, leaving it as is for now.

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.

Add option to configure a custom reranker in OCL Mapper Project Config

3 participants