sublayerapp / sublayer

A model-agnostic Ruby Generative AI DSL and framework. Provides base classes for building Generators, Actions, Tasks, and Agents that can be used to build AI powered applications in Ruby.
https://docs.sublayer.com
MIT License
119 stars 2 forks source link

format properties in output adapters #48

Closed AndrewBKang closed 4 months ago

AndrewBKang commented 4 months ago

json/xml formatting is being moved from providers to output adapters.

simplifies the implementation of the formatting methods (no if/else/cases) simplifies the providers to just focus on the api calls and the response parsing.

swerner commented 4 months ago

Yeah, this is similar to the previous implementation of output adapter subclasses that we had back in 0.0: https://github.com/sublayerapp/sublayer/blob/0.0/lib/sublayer/components/output_adapters/single_string.rb

Putting the formatting into the output adapter subclass itself puts a lot more of a burden on people writing the output adapter subclass for something that could ultimately be generated based on the properties (like the output adapter module itself).

I guess we have two options here - go back to this 2 method xml/json formatted properties method and in that case we can just remove the properties method inside output adapters...

Or create a universal method on OutputAdapters that formats the openstruct properties into the OpenAPI spec that all the models use...

AndrewBKang commented 4 months ago

The universal one would be nice but new output adapters (custom or in our framework) might require an update to the universal method (as it was for enums)

Because of that I think having each output adapter be responsible for it's formatting is preferred. What do you think?

On another note, I do think that we could get rid of the xml formatted properties with an update to the gemini provider as well to use function calling. which would simplify it further.

random note: I was thinking there's also the option where we could have a base method for these formats that includes the generic pieces (like the output name, description) and have the output adapters handle the other fields. but I didn't like how it looked/felt. it needlessly complicate the interface.

swerner commented 4 months ago

Yeah, I was thinking we'd do a more universal conversion of that OpenStruct structure to OpenAPI spec and store it in the module, that way we don't continue to break backwards compatibility with OutputAdapters.

Ideally we'd build it in a way that we don't need to update it again unless a new tool calling format comes out.

This is just a scratch example, will want to flesh it out with more tests but something like this on OutputAdapters:

 def self.format_properties(props)
    props.map do |prop|
      format_property(prop)
    end
  end

  def self.format_property(prop)
    result = {
      name: prop.name,
      type: prop.type,
      description: prop.description,
      required: prop.required
    }

    case prop.type
    when 'array'
      result[:items] = prop.items.is_a?(OpenStruct) ? format_property(prop.items) : prop.items
    when 'object'
      result[:properties] = format_properties(prop.properties) if prop.properties
    end

    result
  end

The main motivations here are to make it so users don't need to know or write the OpenAPI spec format for their properties, just create the interface and the structs, and also to avoid continuing to break backwards compatibility as we play with this...

swerner commented 4 months ago

Though thinking about it more, I think one doesn't actually preclude the other. By having a format_properties method on OutputAdapters you can always override it in your custom output adapter for anything that isn't yet supported...

swerner commented 4 months ago

But also, yeah in this pr: https://github.com/sublayerapp/sublayer/pull/47/files I'd actually changed the Gemini Provider to use the json-based tool use...so we can probably get rid of the xml one

AndrewBKang commented 4 months ago

But also, yeah in this pr: https://github.com/sublayerapp/sublayer/pull/47/files I'd actually changed the Gemini Provider to use the json-based tool use...so we can probably get rid of the xml one

You already did it awesome! woot~

AndrewBKang commented 4 months ago

Though thinking about it more, I think one doesn't actually preclude the other. By having a format_properties method on OutputAdapters you can always override it in your custom output adapter for anything that isn't yet supported...

I wonder about the extra translation layer with the open struct. Though it is openai's api, the properties are really just json schema.

But let's give it a try. universal method can stay up to date with the latest openai spec and work with the outputadapters that are in the library. And custom ones can have their own format methods when needed.

We can learn as more custom adapters come in.

I'll make the changes

swerner commented 4 months ago

I wonder about the extra translation layer with the open struct.

Yeah that's a fair point...mostly the idea there is to make it so users don't need to write and pass around blobs of json. OpenStruct isn't a huge improvement, but at least its a Ruby object. Then this way we have a public interface and the interface in the right places so we can improve and simplify without having to make changes across the whole framework to add a new param...