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

Update ChatCompletionSummary attribute datatypes #2509

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

kaylareopelle
Copy link
Contributor

@kaylareopelle kaylareopelle commented Mar 13, 2024

While doing an audit on the response header datatypes, I thought I'd check the other event types. It looks like only request.temperature and request.max_tokens needed updates.

Since these values come from the user, we can't be certain of the data type.

In the spec, request.temperature can be a float or an integer. To simplify the logic, I'm always assigning it as a float because a float is more specific than an integer. request.max_tokens must be an integer.

The LLM event tests already use the correct datatypes.

request_max_tokens should be an integer
request.temperature should be a float

This change ensures the correct datatype is assigned.
Copy link
Contributor

SimpleCov Report

Coverage Threshold
Line 93.71% 93%
Branch 71.21% 71%

@kaylareopelle kaylareopelle marked this pull request as ready for review March 13, 2024 23:13
request_model: parameters[:model] || parameters['model'],
temperature: parameters[:temperature] || parameters['temperature'],
temperature: (parameters[:temperature] || parameters['temperature'])&.to_f,
Copy link
Contributor

Choose a reason for hiding this comment

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

@kaylareopelle @hannahramadan sometime later, possibly post-GA, I'd like to circle back to our earlier discussion about this style of essentially performing ActiveSupport::HashWithIndifferentAccess style look-ups. Earlier I was concerned about DRY from the human dev perspective, and now I'm concerned about all of these new string objects being instantiated every time a look-up is performed. It's possible that Ruby is lazy with the string initialization and won't perform it unless the symbol look-up fails and the || or hit. I'd like to better understand the impact and explore our options.

I'm thinking we could do something like this:

class AiClass
  KEYS = %i[max_tokens model temperature].each_with_object({}) do |k, h|
    h[k] = k.to_s.freeze }.freeze
  end

  def ai_method(parameters = {})
    tokens = hash_lookup(parameters, :max_tokens).to_i
    model = hash_lookup(parameters, :model)
    temp = hash_lookup(parameters, :temperature).to_f
    [tokens, model, temp]
  end

  private

  def hash_lookup(hash, key_as_symbol)
    hash[key_as_symbol] || hash[KEYS[key_as_symbol]]
  end
end

ai = AiClass.new
params = {'max_tokens' => 4, model: 'b', 'temperature' => 85}
result = ai.ai_method(params)
pp result

Copy link
Contributor

Choose a reason for hiding this comment

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

James I like this and have capture this in a ticket to revist, most likely post GA!

@hannahramadan hannahramadan merged commit 6ba9084 into openai_instrumentation Mar 18, 2024
29 checks passed
@kaylareopelle kaylareopelle deleted the update-summary-attr-datatypes branch October 9, 2024 19:01
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.

4 participants