Update protobuf definitions for remote list_append fix

[nking-1]

Fix for the issue #920. The issue is that the list_append feature in gen does not work with remote models.

The primary fix here was to update protobuf definitions to support lists inside the captured values. When I generated code from the new protobuf definitions, it resulted in Protobuf automatically using a new version. The old version is 4.25.2 and the new one is 5.27.1. I'm not sure if that is much of a problem or not.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 8.33333% with 66 lines in your changes missing coverage. Please review.

Project coverage is 58.65%. Comparing base (6e4ee06) to head (4d04367).
Report is 1 commits behind head on main.

Files	Patch %	Lines
guidance/models/_model.py	2.77%	35 Missing ⚠️
guidance/_serialization_pb2.py	13.88%	31 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #937      +/-   ##
==========================================
+ Coverage   55.54%   58.65%   +3.11%     
==========================================
  Files          63       63              
  Lines        4733     4775      +42     
==========================================
+ Hits         2629     2801     +172     
+ Misses       2104     1974     -130     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hudson-ai]

Thanks for working on this @nking-1!
With the big caveat that I don't know much about protobuf... this looks reasonable so long as that test is passing :)

@riedgar-ms @paulbkoch @Harsha-Nori can any of you comment? I'm particularly wondering if there is any way to make the serialization/deserialization a bit more "automated" since I'd expect union types to be pretty common use cases?

[riedgar-ms]

The E2E test in the server tests is great, but can we have specific protobuf tests? Basically, create a bunch of grammars and round-trip them through protobuf and show that they're the same? If there's some sort of problem with the protobuf layer, debugging via Server is definitely Not Fun.

[hudson-ai]

@riedgar-ms great idea, but would you consider that required for approval, or can it go on the backlog?

[riedgar-ms]

I'd be happy for a fast-follow. The added test at least flexes the new addition.

[hudson-ai]

@nking-1 this should hopefully pass the gates after #936 is merged. Approved from me and will (conditionally) merge unless anyone objects :)

[nking-1]

Richard raised some concerns about potential backwards compatibility. Let's wait for Michal's review before merging this.

I agree that protobuf-specific tests are important. I can work on that as a followup task.

[hudson-ai]

Got it. @mmoskal could you take a look when you get a chance?

[mmoskal]

is there protobuf used to anything other than the azure_guidance? (which we are replacing with JSON)

[hudson-ai]

@mmoskal just to make a distinction -- there are two protobuf models (protocols?) we are currently using:

1. GrammarFunction
   • Currently used by AzureGuidance and "local" guidance servers (pretty sure that's it)
   • Aim should be to replace both with the JSON version eventually
2. EngineCallResponse
   • The main focus of this PR
   • Currently used only by "local" guidance servers (AzureGuidance is currently a bit nasty and is more manually parsed from JSON embedded in logs -- would be good to standardize on a specific JSON response model from the API and use in both situations?)

[mmoskal]

I see. A common JSON format for response sounds like a good idea!

Given that all this data is processed in Python anyways, I don't think protobuf gives us much of a performance advantage over JSON, especially if we use ujson package or similar.

[hudson-ai]

I think I agree. In the meantime, I don't think there should be any backwards compatibility issues wrt AzureGuidance (I expect that was the primary concern?) as we are changing that whole thing out anyway. @mmoskal do you agree? If so I think we should merge this in the meantime and I can work on replacing protobuf entirely on our feature branch

[mmoskal]

@hudson-ai yes, agreed!