Rename Rhesis Default model to Rhesis - #2385
Conversation
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
Improvement: Tighten the embedding fallback and migration predicates.
get_default_embedding_model()now prefers a more generic name ("Rhesis Embedding")—consider ensuring it can’t accidentally select a user-created model with the same name.- Alembic migration matches only on
name+is_protected; consider additionally matching on expectedmodel_name/provider/model_type so upgrade/downgrade are safer against unexpected duplicates.
| .filter( | ||
| models.Model.organization_id == org_uuid, | ||
| models.Model.name == "Rhesis Default Embedding", | ||
| models.Model.name == "Rhesis Embedding", |
There was a problem hiding this comment.
Improvement: get_default_embedding_model() now prefers the org model named "Rhesis Embedding".
If a user creates a non-protected embedding model with the same name, this will pick that one first (.first()), which seems like an easy footgun now that the name is more generic.
Fix: Consider adding
models.Model.is_protected.is_(True)(and/or joining provider_type == "rhesis") to theby_namequery as well, so the fallback is guaranteed to pick the system model.
There was a problem hiding this comment.
Confirmed fix: adding is_protected.is_(True) to the org-name fallback avoids collisions with user-created models named "Rhesis Embedding". Thanks!
| model_name = 'default', | ||
| description = 'Default Rhesis language model.' | ||
| WHERE name = 'Rhesis' | ||
| AND is_protected = TRUE |
There was a problem hiding this comment.
Improvement: The migration renames rows purely by name + is_protected = TRUE.
If an org ever ends up with multiple protected Rhesis models called "Rhesis" (or "Rhesis Embedding") from prior partial runs/manual fixes, the downgrade() would flip all matching rows back to model_name='default'.
Fix: Consider tightening the WHERE clauses to also include provider (join on
provider_type_id→ type_value = 'rhesis') and the expectedmodel_type, or at least match on the old/newmodel_namevalues too (e.g. upgrade WHEREmodel_name='default', downgrade WHEREmodel_name='rhesis-managed').
There was a problem hiding this comment.
Much safer with the provider_type_id (= rhesis) predicate added. Optional extra hardening: also constrain by the expected model_name (upgrade model_name='default', downgrade model_name='rhesis-managed') and/or model_type, so a future additional protected Rhesis model named "Rhesis" doesn’t get flipped on downgrade.
There was a problem hiding this comment.
Leaving as-is for now — name + is_protected + provider=rhesis already uniquely identifies these rows given the current data model (one protected rhesis-provider model per org per type today). Adding a model_name guard would only matter for a hypothetical future protected model also named "Rhesis" with a different `model_name", so not worth the extra complexity right now. Thanks for flagging it though.
There was a problem hiding this comment.
Makes sense — with provider=rhesis + is_protected (and given the current “one protected rhesis model per org per type” invariant), the migration scope is already pretty tight.
If you keep it as-is, one small suggestion would be to add a brief comment in the migration noting that assumption, so future us knows why we didn’t also guard on model_name/model_type. Otherwise I’m good with this.
|
Update after 13c866d:
Remaining (non-blocking) hardening suggestion: in the Alembic migration, also constrain upgrade/downgrade by the expected |
13c866d to
7d73ec5
Compare
There was a problem hiding this comment.
Critical: apps/backend/.../services/organization.py has invalid Python syntax (model_name=\"rhesis\"). This will break the backend.
Improvement: Avoid editing existing Alembic migration files (e.g. 41a9355b3991_...) after they may have been applied; prefer a new migration for the Polyphemus description tweak.
Found 2 issues (1 critical, 1 improvement).
| model_name="default", | ||
| name="Rhesis Default", | ||
| description="Default Rhesis language model.", | ||
| model_name=\"rhesis\", |
There was a problem hiding this comment.
Critical: This is invalid Python syntax: model_name=\"rhesis\" (the backslashes are literal).
Fix: change to
model_name="rhesis"(no backslashes) in both the language + embedding model creation calls.
There was a problem hiding this comment.
Confirmed fixed in latest commits: model_name="rhesis" is now valid model_name="rhesis" in both default model creation calls. Thanks!
| @@ -0,0 +1,142 @@ | |||
| """rename_rhesis_default_model_to_rhesis | |||
There was a problem hiding this comment.
Improvement: This migration now also tweaks the Polyphemus description (and this MR also edits an existing migration 41a9355b3991_...).
If 41a9355b3991 has already shipped/applied anywhere, modifying it can make migrations non-reproducible (DBs that already ran it won’t get the new description).
Fix: Prefer a new migration (or keep the Polyphemus description change only in this new migration) and leave older migration files immutable. At minimum, please confirm
41a9355b3991isn’t in a released chain yet.
| @@ -89,7 +89,7 @@ def upgrade() -> None: | |||
| model_name="default", | |||
There was a problem hiding this comment.
Improvement: Avoid editing existing Alembic revisions after they may have been applied.
If 41a9355b3991 is already in production history, changing this string won’t update existing DBs and can make the migration chain non-reproducible.
Fix: revert this edit and (if the copy needs changing) do it in a new migration or via the new
b7e3a1c9d2f6migration only.
Display name changes from "Rhesis Default"/"Rhesis Default Embedding" to "Rhesis"/"Rhesis Embedding", and model_name from "default" to "rhesis-managed" (composite id becomes rhesis/rhesis-managed). Includes a data migration for existing org rows. Signed-off-by: Harry Cruz <harry@rhesis.ai>
Require is_protected in the embedding-model by-name lookup so a user-created model can't shadow the system default. Add a provider_type join to the rename migration's WHERE clauses so upgrade/downgrade only ever touch rhesis-provider protected rows. Signed-off-by: Harry Cruz <harry@rhesis.ai>
Change model_name from "rhesis-managed" to "rhesis" so the composite id is rhesis/rhesis. Remove "No API key required/needed" from Polyphemus and Rhesis model descriptions and connection test messages. Update the Alembic migration to apply both changes to existing rows. Signed-off-by: Harry Cruz <harry@rhesis.ai>
…ion down_revision The sed-based rename left escaped backslashes in model_name assignments causing a SyntaxError. Also update the migration's down_revision to 491519fd3010 (current main head). Signed-off-by: Harry Cruz <harry@rhesis.ai>
7d73ec5 to
0bce09c
Compare
Signed-off-by: Harry Cruz <harry@rhesis.ai>
There was a problem hiding this comment.
Update on latest commits:
- ✅
organization.pysyntax issue is fixed (model_name="rhesis"now valid in both default model creations).
Improvement: Avoid editing existing Alembic revisions (41a9355b3991_...). Even for copy changes, it can make the migration chain non-reproducible and won’t update DBs that already applied it.
Fix: revert the edit to
41a9355b3991_...and rely on the newb7e3a1c9d2f6migration (or a new migration) for the Polyphemus description update.
Improvement: In b7e3a1c9d2f6_..., the header says Revises: e90c29496562 but down_revision is 491519fd3010.
Fix: update the docstring header to match the actual
down_revision(or vice versa) to avoid confusion.
Improvement: There’s still a seeded default value "rhesis/rhesis-default" in f3a4b5c6d7e8_add_parameters_to_chatbot_request_mapping.py (chatbot example project parameter schema).
Fix: update that default to
"rhesis/rhesis"so new DBs don’t keep pointing at the old model id.
Found 3 issues (0 critical, 3 improvements).
1 participant
Purpose
The Rhesis-hosted default model was named "Rhesis Default" with model_name "default", which reads oddly in the UI now that other Rhesis-hosted models exist (Polyphemus). It's being renamed to "Rhesis" with model_name "rhesis-managed" to read more cleanly, matching the updated design.
What Changed
model_namefrom "default" to "rhesis-managed" for both, so the composite id becomesrhesis/rhesis-managed(wasrhesis/rhesis-default)ModelSettings), SDK defaults (sdk/src/rhesis/sdk/models/defaults.py), the explorer embedding fallback lookup, chatbot app defaults, Helm chart values, and.env.exampleto the new composite idb7e3a1c9d2f6to rename existing org rows in themodeltable (with a downgrade path back to the old names)Additional Context
dev-rhesis-default-polyphemus-url) are left as-is — those are GCP Secret Manager key names for Polyphemus, unrelated to this model's identifier, and changing them needs coordinated infra work.Testing
uv run pytest ../../tests/backend/services/test_organization.py ../../tests/backend/app/test_settings.py ../../tests/backend/routes/test_explorer.py ../../tests/backend/services/explorer/fromapps/backend— 358 passed, 1 skipped (pre-existing skip)uv run pytest ../tests/sdk/models/test_model_factory.py ../tests/sdk/connector/test_serializer_integration.pyfromsdk— 64 passeduv run alembic upgrade head) and confirmed all 131 org rows updated:Rhesis/rhesis-managedandRhesis Embedding/rhesis-managed, with no leftover "Rhesis Default" rows