Skip to content

Conversation

@jonallured
Copy link
Contributor

Over on Roast I was interested in adding some logic to skip creating an api client when one has been provided via an initializer so I came here to see what that would look like. Turns out it's pretty easy so I thought I'd see about making this change up here.

@obie
Copy link
Contributor

obie commented Jun 2, 2025

@jonallured can you revisit this PR when you get a chance please?

@jonallured jonallured force-pushed the allow-checking-for-api-client branch from d59a833 to 617ffc4 Compare June 3, 2025 14:59
@jonallured
Copy link
Contributor Author

Ok @obie this has been updated with latest main. Can you approve the workflow so that the CI actually run please? 😝

@obie obie self-requested a review June 5, 2025 20:03
self.fallback = fallback
end

def client?
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using this method inside of Roast to avoid clobbering the client that was being set in an initializer. So the use case was:

  • if the config already has a client, bail
  • if the config does not have a client, then check for an env var and make the right client

That was here Shopify/roast#65 but that PR was closed and the underlying issue was addressed in a different way. That could mean that this PR is also no longer needed - happy to close if you feel like this is not needed!

I did just take a look at how this is working in Roast today - so much has changed since I opened this PR! 😝 But you can see that Roast is pretty knowledgeable about how Raix works:

https://github.com/Shopify/roast/blob/main/lib/roast/workflow/workflow_initializer.rb#L120

What I think is better encapsulation would be for the config to answer the question of "are you already configured with a client?" so that Roast could look more like this:

      def configure_api_client
        # Skip if api client is already configured (e.g., by initializers)
-       return if api_client_already_configured?
+       return if @configuration.client?

WDYT?

Copy link
Contributor

@ujh ujh Jun 6, 2025

Choose a reason for hiding this comment

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

The question is how that interacts with https://github.com/OlympiaAI/raix?tab=readme-ov-file#global-vs-class-level-configuration, where you can already set a client (or any other config option) either on the global level or the class level. Here the one configured on the class level currently takes precedence. I do wonder if that is the same use case, i.e. different clients for different agent classes or if you want something else?

@jonallured jonallured force-pushed the allow-checking-for-api-client branch from 617ffc4 to 3a0ac88 Compare June 6, 2025 13:08
@obie obie merged commit f18ce41 into OlympiaAI:main Jul 10, 2025
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.

3 participants