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

[ClientModel] Validate pattern for returning dual BinaryData/Stream content for large files from protocol methods #43034

Closed
annelo-msft opened this issue Mar 27, 2024 · 3 comments
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. System.ClientModel Base Core library
Milestone

Comments

@annelo-msft
Copy link
Member

In some APIs like DownloadFile in OpenAI client, users sometimes want BinaryData for simplicity, but if the file size is too large to fit in memory, they may need to call the protocol method to use response.ContentStream instead. There are some challenges that come up here, both in general, and for unbranded clients:

  1. Unbranded clients return ClientResult instead of PipelineResponse from protocol methods. ClientResult is not disposable, so the caller can't just put the return value in a using block, they must instead retrieve the response via GetRawResponse and dispose that directly.
  2. Enabling both response.Content and response.ContentStream to be useful is challenging for large data downloads over a network stream. We did a lot of work to mitigate this in ClientModel: Change approach to buffering response.Content #41080, and did not fully complete ClientModel: Investigate the feasibility of providing a tester to know whether PipelineResponse.ContentStream has been read #42049, which may or may not be relevant here. The consideration here is that the caller may need to specify ahead of time which property -- the BinaryData response.Content or the Stream response.ContentStream -- they intend to use so that the client pipeline will know whether to a) buffer it or b) wrap it in a RetriableStream internally instead of buffering it, so that the streamed content will be tolerant to network issues.

A final solution to this would need to make sure it doesn't break the intent of https://gist.github.com/KrzysztofCwalina/da4f27f4513d9c6cc81299b67054416e -- whose goal it was to avoid breaking changes to clients when the return type of a method changes from Response to Repsonse<T> in Azure clients. Or, if it does, that we are ok with that trade-off.

@annelo-msft annelo-msft added Client This issue points to a problem in the data-plane of the library. Azure.Core System.ClientModel Base Core library labels Mar 27, 2024
@annelo-msft annelo-msft added this to the 2024-04 milestone Mar 27, 2024
@annelo-msft
Copy link
Member Author

Related to #43017

@joseharriaga FYI

@jsquire jsquire modified the milestones: 2024-04, 2024-05 Apr 30, 2024
@annelo-msft
Copy link
Member Author

We decided that end-users will set options.BufferResponse to false in cases where they need access to response.ContentStream. Once they do, it is their responsibility to call result.GetRawResponse() and dispose the PipelineResponse that is returned from that call.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. System.ClientModel Base Core library
Projects
None yet
Development

No branches or pull requests

2 participants