Skip to content

Simplify models#2180

Merged
dgageot merged 4 commits intodocker:mainfrom
dgageot:simplify-models
Mar 19, 2026
Merged

Simplify models#2180
dgageot merged 4 commits intodocker:mainfrom
dgageot:simplify-models

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Mar 19, 2026

No description provided.

Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

This PR simplifies the modelsdev package by removing unused fields and streamlining the code structure. The refactoring is clean and maintains the same functionality.

Changes reviewed:

  • ✅ Removed unused fields from Provider and Model structs
  • ✅ Removed unused CachedAt and UpdatedAt timestamps
  • ✅ Converted NewStore to use sync.OnceValues directly (cleaner singleton pattern)
  • ✅ Changed GetProvider to getProvider (unexported, internal use only)
  • ✅ Removed unused ModelSupportsReasoning function
  • ✅ Simplified Bedrock region prefix handling (inlined helper function)
  • ✅ Updated test fixtures to match simplified structs

Concurrency safety: The mutex usage in GetDatabase and getProvider is correct. The cached database is immutable after loading, making concurrent reads safe.

No bugs found in the changed code. The simplification improves code maintainability without introducing issues.

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Can we stop simplifying? It's going in circles at this point

@rumpl
Copy link
Member

rumpl commented Mar 19, 2026

Do we really have to remove the fields? It's a public package and external might need it?

@dgageot dgageot merged commit afd1dfc into docker:main Mar 19, 2026
8 checks passed
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.

2 participants