deprecate LLMs with redirect logic #639
Conversation
|
LGTM 👍 |
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
daras_ai_v2/language_model.py:878
- Recursive redirection in run_language_model may lead to infinite recursion if the redirect target is itself deprecated. Consider adding a safeguard to detect and break potential redirection cycles, e.g. by tracking or limiting redirection depth.
if model.is_deprecated:
daras_ai_v2/language_model.py
Outdated
| else: | ||
| raise UserError(f"Model {model} is deprecated.") |
There was a problem hiding this comment.
this is new behavior for deprecated models without a redirect_to option. raising UserError here.
earlier, the behavior was to attempt to run the model regardless.
|
I wonder if a more robust way to do this would be to define the fallback on the |
daras_ai_v2/language_model.py
Outdated
| if model.is_deprecated: | ||
| if model.redirect_to: |
There was a problem hiding this comment.
why are these 2 nested conditions
There was a problem hiding this comment.
the other way to write would be:
if model.is_deprecated and model.redirect_to:
...
elif model.is_deprecated:
...There was a problem hiding this comment.
sounds good but I dont see an elif condition required in your code
There was a problem hiding this comment.
It is this at the moment:
if model.is_deprecated:
if model.redirect_to:
return run_language_model(...)
else:
raise UserError("model is deprecated")Yes moving to non-nested if-elif will be more obvious
There was a problem hiding this comment.
ah. that doesn't look right. will fix
There was a problem hiding this comment.
fyi: I would have done it like this, but what you have works
if model.is_deprecated:
if not model.redirect_to:
raise UserError(f"Model {model} is deprecated.")
return run_language_model(...)
we can add |
|
|
nikochiko
force-pushed
the
redirect-deprecated-llms
branch
from
9458d79 to
98b1fc3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce a Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
daras_ai_v2/language_model.py(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
daras_ai_v2/language_model.py (1)
daras_ai_v2/exceptions.py (1)
UserError(58-65)
daras_ai_v2/language_model.py
Outdated
| if model.is_deprecated and model.redirect_to: | ||
| return run_language_model( | ||
| prompt=prompt, | ||
| messages=messages, | ||
| model=model.redirect_to, | ||
| max_tokens=max_tokens, | ||
| num_outputs=num_outputs, | ||
| temperature=temperature, | ||
| stop=stop, | ||
| avoid_repetition=avoid_repetition, | ||
| tools=tools, | ||
| response_format_type=response_format_type, | ||
| stream=stream, | ||
| audio_url=audio_url, | ||
| audio_session_extra=audio_session_extra, | ||
| ) | ||
| elif model.is_deprecated: | ||
| raise UserError(f"Model {model} is deprecated.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Recursive redirect omits quality & risks infinite loops
qualityis accepted byrun_language_model()but not forwarded, so calls that relied on a non-default quality silently regress to1.0.- A mis-configured
redirect_tothat points back to the same (or another already-visited) model will cause infinite recursion.
- return run_language_model(
+ # keep original call semantics and break potential cycles
+ if model.redirect_to == model.name:
+ raise UserError(f"Redirect cycle detected for model {model}")
+ return run_language_model(
prompt=prompt,
messages=messages,
model=model.redirect_to,
max_tokens=max_tokens,
+ quality=quality,
num_outputs=num_outputs,
temperature=temperature,
stop=stop,
avoid_repetition=avoid_repetition,
tools=tools,
response_format_type=response_format_type,
stream=stream,
audio_url=audio_url,
audio_session_extra=audio_session_extra,
)Please forward all user-supplied params (or use locals() with explicit filtering) and add a small visited-set/guard to prevent recursion loops.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if model.is_deprecated and model.redirect_to: | |
| return run_language_model( | |
| prompt=prompt, | |
| messages=messages, | |
| model=model.redirect_to, | |
| max_tokens=max_tokens, | |
| num_outputs=num_outputs, | |
| temperature=temperature, | |
| stop=stop, | |
| avoid_repetition=avoid_repetition, | |
| tools=tools, | |
| response_format_type=response_format_type, | |
| stream=stream, | |
| audio_url=audio_url, | |
| audio_session_extra=audio_session_extra, | |
| ) | |
| elif model.is_deprecated: | |
| raise UserError(f"Model {model} is deprecated.") | |
| if model.is_deprecated and model.redirect_to: | |
| # keep original call semantics and break potential cycles | |
| if model.redirect_to == model.name: | |
| raise UserError(f"Redirect cycle detected for model {model}") | |
| return run_language_model( | |
| prompt=prompt, | |
| messages=messages, | |
| model=model.redirect_to, | |
| max_tokens=max_tokens, | |
| quality=quality, | |
| num_outputs=num_outputs, | |
| temperature=temperature, | |
| stop=stop, | |
| avoid_repetition=avoid_repetition, | |
| tools=tools, | |
| response_format_type=response_format_type, | |
| stream=stream, | |
| audio_url=audio_url, | |
| audio_session_extra=audio_session_extra, | |
| ) | |
| elif model.is_deprecated: | |
| raise UserError(f"Model {model} is deprecated.") |
|
Also:
|
this is what i did initially, but the way the |
… suite to verify that all deprecated models have a valid redirect and that the redirected models are not deprecated.
devxpy
force-pushed
the
redirect-deprecated-llms
branch
from
ea43a8f to
087a79a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_llm.py (2)
8-9: Consider validating the redirect_to attribute typeThe test correctly verifies that
redirect_tois not empty, but it doesn't validate that it's a string type. While this may be implicitly tested in assertion #2, an explicit type check would make the test more robust.- assert model.redirect_to, f"{model.name} is deprecated but has no redirect_to" + assert model.redirect_to and isinstance(model.redirect_to, str), f"{model.name} is deprecated but has no redirect_to or redirect_to is not a string"
16-20: Consider adding a test for multi-level redirectsThe current implementation verifies that immediate redirects don't point to deprecated models, but it doesn't check for potential chained/cascading redirects in the future (if model A redirects to B, and later B becomes deprecated and redirects to C).
While not critical for the current implementation, a utility function that follows redirects to their final destination could be useful both for testing and potentially for the main codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/language_model.py(32 hunks)tests/test_llm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- daras_ai_v2/language_model.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_llm.py (1)
daras_ai_v2/language_model.py (1)
LargeLanguageModels(94-807)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (1)
tests/test_llm.py (1)
4-20: Good test implementation for model redirection!This test function thoroughly checks the three key requirements for the deprecation redirect logic:
- All deprecated models have a redirect target
- Redirect targets exist in the enum
- Redirect targets are not themselves deprecated
This will prevent circular redirects and ensure all deprecated models have valid replacements.
Q/A checklist
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.