Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raise error: instead of defaulting any model #2100

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Prabesh1sharma
Copy link

@Prabesh1sharma Prabesh1sharma commented Feb 13, 2025

Description

  • Summary of changes: Removed automatic fallback to OpenAI model and enforced explicit model specification

  • Related issues: fixes

  • Motivation and context: Given that Agno is designed to support multiple models, forcing OpenAI as the default model is not ideal.

    • Ensures intentional model selection by users
    • Improves error messaging for missing configuration
    • Increases framework flexibility by removing default model assumption
  • Environment or dependencies: Removes implicit requirement for openai package

  • Impact on metrics: N/A (configuration change rather than model performance change)

Fixes # (issue)


Type of change

Please check the options that are relevant:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Model update (Addition or modification of models)
  • Other (please describe):

Checklist

  • Adherence to standards: Code complies with Agno’s style guidelines and best practices.
  • Formatting and validation: You have run ./scripts/format.sh and ./scripts/validate.sh to ensure code is formatted and linted.
  • Self-review completed: A thorough review has been performed by the contributor(s).
  • Documentation: Docstrings and comments have been added or updated for any complex logic.
  • Examples and guides: Relevant cookbook examples have been included or updated (if applicable).
  • Tested in a clean environment: Changes have been tested in a clean environment to confirm expected behavior.
  • Tests (optional): Tests have been added or updated to cover any new or changed functionality.

Additional Notes

Include any deployment notes, performance implications, security considerations, or other relevant information (e.g., screenshots or logs if applicable).

@Prabesh1sharma Prabesh1sharma requested a review from a team as a code owner February 13, 2025 07:17
Copy link
Contributor

@Harsh-2909 Harsh-2909 left a comment

Choose a reason for hiding this comment

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

The code looks good to me.
But this is a breaking change as it will break a few already working Agents.

I think we should release it in a minor or major release.

)
exit(1)
self.model = OpenAIChat(id="gpt-4o")
logger.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably make the model parameter of the class as required instead of Optional.
Also this will break tons of code which do not provide model when creating Agent (like most of the examples in cookbook/tools section)
I think we should create a discussion in the Issues to talk about it. Also, if it is merged, it should be released in the next major patch as it is a breaking change.

Copy link
Author

@Prabesh1sharma Prabesh1sharma Feb 13, 2025

Choose a reason for hiding this comment

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

We could take that approach, but for now, I believe it's one possible solution. A framework shouldn't enforce OpenAI's model as the default option. Instead, it should either require users to explicitly define a model or offer a more flexible default configuration.
we can have fruitful discussion on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

True that, we should not have default pointing to a single vendor.
We can probably have a default config where we can define the default model behaviour. Like whether to raise an error in case the model is not provided or use a default model (which can be configured there). That will give lots of flexibility to the users of Agno. Something like a .agno_config file or similar.
Again, would be good to discuss this further.

Copy link
Author

Choose a reason for hiding this comment

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

I think if we do that, there’s no point in having these kinds of updates if you still need to configure it. We can include the model in every agent, and we won’t face these issues. My point is that we shouldn’t default to any single vendor. Instead, we can raise an error if the model isn’t provided. After making this change, we’ll only need to make small adjustments across the codebase, and it won’t take much time.

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