Skip to content

fix: make model metadata merging deterministic - #7874

Open
harsh21234i wants to merge 3 commits into
reflex-dev:mainfrom
harsh21234i:fix/7830-deterministic-model-metadata
Open

harsh21234i wants to merge 3 commits into
reflex-dev:mainfrom
harsh21234i:fix/7830-deterministic-model-metadata

Conversation

@harsh21234i

@harsh21234i harsh21234i commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #7830 by preserving model registration order when combining metadata from multiple model bases. If separate metadata objects define the same table, the registry now raises a clear InvalidRequestError instead of silently selecting one definition.

Regression coverage verifies deterministic ordering through register(), ordering after a model is removed and re-registered, duplicate table detection, and preservation of convention-generated constraint names.

Testing:

  • uv run pytest tests/units/test_model.py tests/units/utils/test_migrations.py -q — 60 passed.
  • Ruff format/check, codespell, and Pyright passed for the changed files.
  • The full unit suite on Windows reported 15,064 passed, 85 skipped, and 21 failures in platform-sensitive/unrelated tests (including symlink privilege limitations).

View guided diff

@harsh21234i
harsh21234i requested a review from a team as a code owner November 12, 2026 19:55
@greptile-apps

greptile-apps Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The follow-up change appears safe to merge.

Summary

The PR makes database table merging predictable and rejects conflicting table definitions.

  • Model metadata follows registration order and a stable order for detected bases.
  • Metadata merging names both model bases when separate bases claim one table.

The earlier test concern is addressed: the ordering test now calls register(). No new actionable issues were found in the follow-up change.

Reviews (3) · Last reviewed commit: "perf: avoid scanning model registry on r..." · Reviewed by Greptile

Comment thread tests/units/test_model.py Resolved

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread reflex/model.py Resolved
Comment thread tests/units/test_model.py Resolved
Comment thread tests/units/test_model.py Resolved
@codspeed

codspeed Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 17 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 143 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing harsh21234i:fix/7830-deterministic-model-metadata (701bd6d) with main (8e5bb84)

Open in CodSpeed

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@FarhanAliRaza FarhanAliRaza added the bug Something isn't working label Nov 13, 2026 — with ChatGPT Codex Connector

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merged model metadata: a table defined in two bases is picked at random per process, and the base's naming_convention is dropped

2 participants