Skip to content

Comments

Allow initializer to set Raix client#65

Closed
jonallured wants to merge 1 commit intomainfrom
allow-initializer-to-set-raix-client
Closed

Allow initializer to set Raix client#65
jonallured wants to merge 1 commit intomainfrom
allow-initializer-to-set-raix-client

Conversation

@jonallured
Copy link
Contributor

As of #56 we had a little regression where an initializer that set the Raix api client would get clobbered by the configure_api_client method because the guard was no longer short circuiting. What this PR does is establish in a test that when Raix.configuration has either an openrouter_client or an openai_client then we should bail on the configure_api_client method.

I did this by checking the state of the Raix.configuration but I also opened OlympiaAI/raix#25 because it would be cool for this configuration object to answer the question of whether it has a client. LMK if there's a preferences on whether to merge this as-is or to merge that and then use that solution instead!

Copy link
Contributor

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This fixes the issue.

Consider a different pattern in this code to make it cleaner?

def client
    @client ||= Raix.configuration.openai_client || Raix.configuration.openrouter_client || configure_api_client
end

def configure_api_client
    raise MissingToken unless configuration.api_token

   ...
rescue 
    "Error configuring ..."
end 

@jonallured
Copy link
Contributor Author

My take on this is that since the client is state on the Raix::Configuration class and is NOT stored inside Roast then we should defer to that class which is why I opened a PR over on Raix. This PR as-is is a compromise. It fixes the regression I saw but I do think merging OlympiaAI/raix#25 and then using that method is nicer.

If we did that then this PR would look more like this:

def configure_api_client
  return unless configuration.api_token
  return if Raix.configuration.client?

  # ...
end

Open to other thoughts though - I'm new here. 😉

@dblock
Copy link
Contributor

dblock commented May 22, 2025

I am good with this change @jonallured, will let the maintainers merge.

@obie
Copy link
Contributor

obie commented May 26, 2025

Hi @jonallured, thank you for catching this regression and taking the time to submit a fix! 🙏

Good news - this issue has already been addressed in the current codebase. The WorkflowInitializer now includes a check via the api_client_already_configured? method that prevents overwriting existing API clients set by initializers. This achieves the same goal as your PR but with a more structured, provider-specific approach.

You can see the implementation in lib/roast/workflow/workflow_initializer.rb lines 39 and 73-82.

Thanks again for your contribution and for helping make Roast better! Your vigilance in catching this regression was much appreciated.

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