Feat: Ruby SDK - initial implementation, tests, and documentation#1
Feat: Ruby SDK - initial implementation, tests, and documentation#1
Conversation
efe557e to
1334710
Compare
docs/sdk/ruby/encoder/overview.mdx
Outdated
|
|
||
| | Parameter | Type | Description | | ||
| | --- | --- | --- | | ||
| | `accept` | `String` (optional) | Content type accepted by the client | |
There was a problem hiding this comment.
La descripción más arriba dice que emite SSE. Si solo emitimos SSE, el accept siempre seria application/json o no?
There was a problem hiding this comment.
Tu puede responder application/json si no va enviar eventos, responder directo o puede hacerlo por text/event-stream que es por evento.
| # @return [String, Object] | ||
| sig { params(event: T.untyped).returns(T.untyped) } | ||
| def convert_payload_to_sse(event) | ||
| if event.respond_to?(:as_json) |
There was a problem hiding this comment.
Por que aca aceptas as_json, pero en los otros lados usas normalize_value que no? Creo que conviene ser consistente
There was a problem hiding this comment.
normalize_value es para transformar en un hash a partir del método to_h definido en cada tipo de clase. Ya as_json es para converter en en JSON, pero en el formato esperado por el protocolo, más allá de simplesmente converter en json (This converts keys to camelCase and removes nil values recursively).
There was a problem hiding this comment.
Entonces si paso un hash no se van a convertir los keys a camelCase... es eso lo esperado?
encoder.encode({ bla_bla: true })
# => "data: {\"bla_bla\": true}\n\n"
encoder.encode(OpenStruct.new(bla_bla: true)
# => "data: {"blaBla": true}\n\n"Esto pasará porque Hash responde a as_json
There was a problem hiding this comment.
No había visto que Model define as_json. Encuentro raro que la misma transformación la hagamos en 2 partes.
There was a problem hiding this comment.
Es verdad, esa parte lo hizo la IA, ahora que veo se quedó redundante si todo hereda de Model. Quité el método y ocupo directamente event.as_json.
| acc[k] = compacted | ||
| end | ||
| when Array | ||
| arr = value.map { |v| deep_compact(v) }.reject(&:nil?) |
There was a problem hiding this comment.
Esto genera 2 arrays, usando reject! eliminas la segunda copia.
There was a problem hiding this comment.
Eso es para eliminar los valores que quedan en nil para evitar tener propiedades en nil.
There was a problem hiding this comment.
Sí, a lo que me refiero es que este codigo es equivalente a:
tmp1 = value.map{ |v| deep_compact(v) }
tmp2 = value.reject(&:nil?)
arr = tmp2Donde tmp1 y tmp2 son objetos distintos, con su asignación de memoria independiente (tmp1.object_id != tmp2.object_id). Lo que propongo es:
tmp1 = value.map{ |v| deep_compact(v) }
tmp1.reject!(&:nil?)
arr = tmp1Lo que nos ahorra el generar un array. (ahora que lo releo, podría ser compact! tambien)
Puede que sea una optimización irrelevante, pero no se de que tamaño son los objetos que estarás generando.
| # @return [String] | ||
| sig { params(event: T.untyped).returns(String) } | ||
| def encode_sse(event) | ||
| payload = convert_payload_to_sse(event) |
There was a problem hiding this comment.
esto en realidad es convirtiendo a json, no a sse.
There was a problem hiding this comment.
Nop, es SSE, ese formato "data: #{json_data}\n\n" es lo que se espera en el protocolo.
There was a problem hiding this comment.
Pero el data: lo agregas despues. No lo haces dentro de convert_payload_to_sse
There was a problem hiding this comment.
Ahora entendí lo que quiere decir, es el nombre del método convert_payload_to_sse . Tan poco es JSON, porque genera después igual, en la misma linea del SSE. Quité el método porque todo viene de Model y siempre van tener as_json. Agregué el tipo también a event para garantizar eso.
| description: "Documentation for the events used in the Agent User Interaction Protocol (Ruby SDK)" | ||
| --- | ||
|
|
||
| # Events |
There was a problem hiding this comment.
Esto habrá que mantenerlo sincronizado con el codigo, pues es basicamente la docu del modulo Events. No podremos generarlo desde el yard? Idem para los otros docs
There was a problem hiding this comment.
Lo voy hacer con yard-markdown.
There was a problem hiding this comment.
Listo documentación creada con yard-markdown para mantener siempre actualizada con la documentación del modulo.
Basta ejecutar rake doc que actualiza los markdowns de acuerdo a la documentación de Ruby.
1334710 to
69c1076
Compare
| # @return [String] | ||
| sig { params(event: T.untyped).returns(String) } | ||
| def encode_sse(event) | ||
| payload = convert_payload_to_sse(event) |
There was a problem hiding this comment.
Pero el data: lo agregas despues. No lo haces dentro de convert_payload_to_sse
| # @return [String, Object] | ||
| sig { params(event: T.untyped).returns(T.untyped) } | ||
| def convert_payload_to_sse(event) | ||
| if event.respond_to?(:as_json) |
There was a problem hiding this comment.
Entonces si paso un hash no se van a convertir los keys a camelCase... es eso lo esperado?
encoder.encode({ bla_bla: true })
# => "data: {\"bla_bla\": true}\n\n"
encoder.encode(OpenStruct.new(bla_bla: true)
# => "data: {"blaBla": true}\n\n"Esto pasará porque Hash responde a as_json
| acc[k] = compacted | ||
| end | ||
| when Array | ||
| arr = value.map { |v| deep_compact(v) }.reject(&:nil?) |
There was a problem hiding this comment.
Sí, a lo que me refiero es que este codigo es equivalente a:
tmp1 = value.map{ |v| deep_compact(v) }
tmp2 = value.reject(&:nil?)
arr = tmp2Donde tmp1 y tmp2 son objetos distintos, con su asignación de memoria independiente (tmp1.object_id != tmp2.object_id). Lo que propongo es:
tmp1 = value.map{ |v| deep_compact(v) }
tmp1.reject!(&:nil?)
arr = tmp1Lo que nos ahorra el generar un array. (ahora que lo releo, podría ser compact! tambien)
Puede que sea una optimización irrelevante, pero no se de que tamaño son los objetos que estarás generando.
| value.each_with_object({}) do |(k, v), acc| | ||
| next if v.nil? | ||
| compacted = deep_compact(v) | ||
| next if compacted.nil? | ||
| acc[k] = compacted | ||
| end |
There was a problem hiding this comment.
| value.each_with_object({}) do |(k, v), acc| | |
| next if v.nil? | |
| compacted = deep_compact(v) | |
| next if compacted.nil? | |
| acc[k] = compacted | |
| end | |
| value.transform_values { |v| deep_compact(v) unless v.nil? }.tap(&:compact!) |
Mi obsesión por las asignaciones de memoria continúa jaja. (este tiene la ventaja de que no hace crecer el hash lo que puede ser que hagas varias asignaciones, pide una del tamaño máximo que podría tener el hash)
| require "test_helper" | ||
| require "json" | ||
|
|
||
| class EventsTest < Minitest::Test |
There was a problem hiding this comment.
Aca deberian haber tests de que el mensaje se codifica de manera exacta o no? Estas solo validando un atributo por evento (el type)
| # @return [Hash<Symbol, Object>] | ||
| sig { returns(T::Hash[Symbol, T.untyped]) } | ||
| def to_h | ||
| {} |
There was a problem hiding this comment.
Como esta es una clase abstracta, debería ser raise NotImplementedError creo yo
| # @return [String, Object] | ||
| sig { params(event: T.untyped).returns(T.untyped) } | ||
| def convert_payload_to_sse(event) | ||
| if event.respond_to?(:as_json) |
There was a problem hiding this comment.
No había visto que Model define as_json. Encuentro raro que la misma transformación la hagamos en 2 partes.
| { | ||
| type: @type, | ||
| timestamp: @timestamp, | ||
| raw_event: @raw_event |
There was a problem hiding this comment.
porque no retornamos altiro los keys camelCased?
There was a problem hiding this comment.
Porque en Ruby normalmente no escribimos así. En la versión de Python igual convierte después, porque quien le gusta así es el front.
3001f9e to
c81f4ad
Compare
|
Aplicado todos los cambios solicitados. |
92a7532 to
540a5f5
Compare
2e3f4b8 to
570279b
Compare
This PR introduces the Ruby SDK for the AG‑UI (Agent‑User Interaction) Protocol, including the initial SDK implementation, a test suite, and reference documentation pages.
The goal is to provide Ruby building blocks for:
What changed:
Tests:
Docs: