protocol models responses

[christothes]

No description provided.

[KrzysztofCwalina]

This is very nice and close. I added 3 comments, but supper happy to see how this does not complicate the namespace too much (or at all)

[joseharriaga]

public virtual ClientResult<ResponseItemList> GetResponseInputItems(GetResponseInputItemsOptions options = default, CancellationToken cancellationToken = default);
public virtual CollectionResult<ResponseItem> GetResponseInputItems(string responseId, ResponseItemCollectionOptions options = null, CancellationToken cancellationToken = default);
public virtual CollectionResult GetResponseInputItems(string responseId, int? limit, string order, string after, string before, RequestOptions options);
public virtual Task<ClientResult<ResponseItemList>> GetResponseInputItemsAsync(GetResponseInputItemsOptions options = default, CancellationToken cancellationToken = default);
public virtual AsyncCollectionResult<ResponseItem> GetResponseInputItemsAsync(string responseId, ResponseItemCollectionOptions options = null, CancellationToken cancellationToken = default);
public virtual AsyncCollectionResult GetResponseInputItemsAsync(string responseId, int? limit, string order, string after, string before, RequestOptions options);        

Looking at the above, I'm a little conflicted about having six methods for each list operation. Additionally, I think it's a little odd that the protocol method has pagination conveniences, while the convenience method that takes protocol models does not have pagination conveniences (it's like neither variant is fully protocol nor fully convenience). I wonder if we can simplify this...

What would you think of the following: What if we do not add list methods that return a protocol model? 99% of users won't benefit from these, and they would instead favor the methods that can paginate. If necessary, I would be in favor of shipping the protocol model by itself to support MRW scenarios and server scenarios.

[christothes]

I think the protocol model return types give better control over advanced pagination scenarios, but I agree they are probably needed rarely. I do think there is some value in having access to the raw protocol model without the pagination abstractions.

[joseharriaga]

@KrzysztofCwalina What is the story for users growing from paginating protocol methods to convenience methods?

[joseharriaga]

We discussed this offline and arrived at the design described here:
🔗 https://gist.github.com/joseharriaga/8c9c57eca935cbe104477b59b0bce6dc

[KrzysztofCwalina]

@m-nash, it would be good to change this into your new ctor pattern before we GA, i.e. not necessarily in this PR.

[joseharriaga]

In the OpenAI library, we started using this pattern of separating the required properties from the optional properties for a few reasons:

1. Users found it awkward to have use two different ways to set the properties of the options class: They had to use the constructor for required properties, and initialization syntax for the optional properties. The options classes in the OpenAI API usually have tons of properties, and it is very common to at least want to set one or two. So this issue was very front-and-center. By making the options class all optional properties, this pain point was mitigated.
2. Users wanted to re-use an instance of the options class across requests. Consider the following scenario: When using stateful responses, if the input items are in the options class, users need to clear the collection and then re-populate it with the new input items every time. By moving the input items out of the options class, users can forget about the options instance entirely.

Would we want to preserve this functionality?

What if we kept the convenience methods as they are right now?

Here's an example of what I'm thinking...

We keep the following method:

public virtual ClientResult<OpenAIResponse> CreateResponse(IEnumerable<ResponseItem> inputItems, ResponseCreationOptions options = null, CancellationToken cancellationToken = default);

And then, we add a parameterless constructor to the options class. If a user specifies inputItems in both, the method parameter and the options property, we throw an exception (similar to what we would do if they set the stream property but call the non-streaming method).

[joseharriaga]

We discussed this offline. For now, we're going with the design described here:
🔗 https://gist.github.com/joseharriaga/8c9c57eca935cbe104477b59b0bce6dc

[joseharriaga]

@KrzysztofCwalina What is the story for users growing from paginating protocol methods to convenience methods?