-
-
Notifications
You must be signed in to change notification settings - Fork 59
Version 1 Refactor #241
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
base: main
Are you sure you want to change the base?
Version 1 Refactor #241
Conversation
a34f084
to
41c57d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves provider gem loading error messages and introduces structured type safety for provider options configuration, particularly for OpenRouter. It also fixes issues with OpenRouter's model/models fallback construction and max price parameter naming.
- Replaces manual gem loading with a centralized
require_gem!
helper - Introduces typed options classes to improve maintainability and type safety
- Fixes OpenRouter provider configuration and parameter handling
Reviewed Changes
Copilot reviewed 32 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
lib/active_agent/generation_provider/_base_provider.rb | Adds centralized gem loading helper and updates base class name |
lib/active_agent/generation_provider/open_ai_provider.rb | Refactors to use new Options class for configuration |
lib/active_agent/generation_provider/open_router_provider.rb | Major refactor to use structured options and fix parameter handling |
lib/active_agent/generation_provider/open_router/*.rb | New structured options classes for type safety |
lib/active_agent/generation_provider/ollama_provider.rb | Updates to use new Options class |
test/ files | Updates tests to work with new structured configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lib/active_agent/providers/open_router/requests/provider_preferences.rb
Outdated
Show resolved
Hide resolved
c6a47d1
to
5dcece4
Compare
5dcece4
to
c749f42
Compare
3f5a803
to
68f98d1
Compare
68f98d1
to
d1f3eec
Compare
c743d7e
to
5b5b17c
Compare
ca9bd26
to
76ee1c7
Compare
857eebd
to
a6f74c2
Compare
anthropic ~> 1.12
(fromruby-anthropic ~> 0.4.2
)any
andtool
usageon_stream_open
andon_stream_close
events. Each callback can take an optional StreamChunk.openai
/open_ai
andopenrouter
/open_router
provider loadingArchitecture
The prior version, through iteration, had a lot of places where functionally the same parameters merge process was happening, that would sometimes call on themselves in self referential loops. These weak boundaries make the maintenance challenging while leading to a number of bugs where these parameters would not take as expected.
This PR move to simplify this process into 3 discrete steps:
Collect -> [Merge -> Translate]
. Collection is separated out to defer the merge & translation steps as late as possible to maximize the available context for making translation decisions, like in the case of OpenAI determining which version of the API to use based on audio support.Merge & Translate happen in series, while the domains of concerns are split. Each layer of options is merged into request builder thus transferring their order of operations while the request builder itself is responsible for translating from the input formats to the output request payload.
Note:
to_hc
is built uponto_h
but adds additional compression by removing default valuesNotes & Considerations
There are a lot of dead code paths. I'm assuming a lot of this has been generated by an LLM based on how much of the code base is not executable. This is confounded by the tests mainly breaking interfaces and calling private methods, which causes these dead code paths to be tested, creating a false sense of security.
Rails 7.2 is EOL, which means that this largely is targeting Rails 8. Rails 8 has a minimum Ruby version of 3.1. Partly to aid in the speed of my development, focused on 3.1 support. I can reintroduce backwards support by request.
A lot of the methods seem near random ordering within the files, which makes large files like
ActionPrompt::Base
hard to grok. As part of working through things I've worked to resort methods to be in call stack order.Part of the challenge with work with this gem is the layers of indirection between the provider API and the language that the gem creates itself, with a leaky abstraction. Since this abstraction is leaky, and is basically required to be leaky in order to keep pace with the provider APIs, while rebuilding the internal interfaces my goal was to preserve the ability to send prompt messages/input/parameters in the native format unmolested then layer on the support "Common" format appropriate. This should make it easier for people to migrate to ActiveAgents while allowing the "Common" format to develop un-rushed by attempting to provide support for in-flight feature sets and patterns from the providers.
TODO