Skip to content

Feat integrate openrouter#67

Merged
onur-askui merged 4 commits intomainfrom
feat/integrate-openrouter
Jun 6, 2025
Merged

Feat integrate openrouter#67
onur-askui merged 4 commits intomainfrom
feat/integrate-openrouter

Conversation

@onur-askui
Copy link
Contributor

No description provided.

@onur-askui onur-askui marked this pull request as ready for review June 5, 2025 13:04
@onur-askui onur-askui requested a review from adi-wan-askui June 5, 2025 13:04
@onur-askui onur-askui force-pushed the feat/integrate-openrouter branch from 730310e to d27808d Compare June 5, 2025 15:00
Copy link
Contributor

@adi-wan-askui adi-wan-askui left a comment

Choose a reason for hiding this comment

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

Super well done (not overengineered, extendable for the most part and well-documented) :) Could not find any blockers from merging, only optimisations I would do for e.g., more consistency or ease of use... (could also be tackled in follow up PR so we get this out fast)

Comment on lines +66 to +68
if response_schema is not None:
error_msg = f'Response schema is not supported for model "{model_choice}"'
raise NotImplementedError(error_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we easily support this (see https://openrouter.ai/docs/features/structured-outputs#best-practices) by setting require_parameters to True if response_schema is not None or isinstance(response_schema, str)? I guess if the model does not support it then, it is going to raise an exception and in all other cases route to a model that supports it.

@onur-askui onur-askui force-pushed the feat/integrate-openrouter branch from 2b092dd to 7a6b68d Compare June 6, 2025 13:12
@onur-askui
Copy link
Contributor Author

Super well done (not overengineered, extendable for the most part and well-documented) :) Could not find any blockers from merging, only optimisations I would do for e.g., more consistency or ease of use... (could also be tackled in follow up PR so we get this out fast)

As discussed, will address the comments in a separate PR

@onur-askui onur-askui merged commit 0d28879 into main Jun 6, 2025
1 check passed
@onur-askui onur-askui deleted the feat/integrate-openrouter branch June 6, 2025 13:24
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