Skip to content

Conversation

@SongChiYoung
Copy link
Contributor

@SongChiYoung SongChiYoung commented Mar 31, 2025

Why are these changes needed?

This PR improves how model_family is resolved when selecting a transformer from the registry.
Previously, model families were inferred using a simple prefix-based match like:

if model.startswith(family): ...

This works for cleanly prefixed models (e.g., gpt-4o, claude-3) but fails for models like mistral-large-latest, codestral-latest, etc., where prefix-based matching is ambiguous or misleading.

To address this:
• model_family can now be passed explicitly (e.g., via ModelInfo)
• _find_model_family() is only used as a fallback when the value is "unknown"
• Transformer lookup is now more robust and predictable
• Example integration in to_oai_type() demonstrates this pattern using self._model_info["family"]

This change is required for safe support of models like Mistral and other future models that do not follow standard naming conventions.

Related issue number

Linked to discussion in #6151
Related : #6011

Checks

@SongChiYoung SongChiYoung force-pushed the Refactor/modelfamily_resolution_to_support_non-prefixed_names_like_Mistral branch from 2944bae to 6f7bd0a Compare March 31, 2025 14:11
@codecov
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.33%. Comparing base (9143e58) to head (8397958).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...n-ext/src/autogen_ext/models/openai/_model_info.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6158      +/-   ##
==========================================
+ Coverage   77.32%   77.33%   +0.01%     
==========================================
  Files         197      197              
  Lines       13797    13808      +11     
==========================================
+ Hits        10669    10679      +10     
- Misses       3128     3129       +1     
Flag Coverage Δ
unittests 77.33% <95.45%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SongChiYoung SongChiYoung mentioned this pull request Apr 1, 2025
@SongChiYoung
Copy link
Contributor Author

@ekzhu First of all, Done.

🙋‍♂️ Should fallback raise branches be covered by tests for Codecov?
Just in case, I added tests — but want to confirm if it’s required for future PRs.
(However I could not add warnings.warn test case, So, if we need to test it, we need to add to _MODEL_INFO only for testcase)

⚠️ Saw a Codecov GPG signature error.
Pretty sure it’s not from my side 😅 — just checking!

@SongChiYoung SongChiYoung requested a review from ekzhu April 2, 2025 11:17
@ekzhu ekzhu enabled auto-merge (squash) April 2, 2025 21:42
@SongChiYoung
Copy link
Contributor Author

@ekzhu Sorry… Before the merge, there was no formatting error on my PC, but after the merge, one error was added (automatically resolved).
I didn’t notice it because it was auto-resolved silently. ;(

So it’s now resolved.

Ehhhhhh… Thank you for fixing it!
When I pushed my code, I realized that you had already fixed it.

FYI: My typical contribution window is KST 9PM–2AM and 6AM–9AM.
I may be slower to respond during KST 9AM–7PM, except for urgent issues.
Just wanted to share this for better async coordination. 🙇

@ekzhu ekzhu merged commit 27da37e into microsoft:main Apr 2, 2025
57 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