Skip to content

Feature/model reader writer#39421

Merged
m-nash merged 16 commits intofeature/core-experimentfrom
feature/modelReaderWriter
Oct 20, 2023
Merged

Feature/model reader writer#39421
m-nash merged 16 commits intofeature/core-experimentfrom
feature/modelReaderWriter

Conversation

@m-nash
Copy link
Copy Markdown
Member

@m-nash m-nash commented Oct 20, 2023

Adds public APIs to be able to read and write models in multiple formats.

Replacement for #38506

Contributing to the Azure SDK

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

/// The format is determined by the implementer.
/// </summary>
/// <typeparam name="T">The type the model can be converted into.</typeparam>
public interface IModel<out T>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to @KrzysztofCwalina on where we want to land with names prior to the review with the BCL team and feedback we might get with them, but I wanted to document some observations I have (non-blocking for this PR):

  • Could the IModel name be confused with the package name ClientModel?
    • Questions on this might include: Why are we modifying the name Model in the package name but not in this interface name?
  • What does the word "model" mean in BCL APIs like ClientModel and ServiceModel and elsewhere, and is this use of the word model consistent with that vocabulary and meaning?
  • Where we also have IJsonModel - is the model itself about Json? Why is Json the best modifier for Model?

One alternative proposal I might toss out there as a starting point for conversation -- and motivated by the observation that .NET interfaces are often used to describe the behaviors enable (per .NET not supporting multiple inheritance), i.e. IEnumerable and IDisposable vs. the "is-a" style interfaces like IList and ICollection -- would be what about something like IContentReadWriteable and IJsonContentReadWriteable?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional comment here is that the "is-a" interfaces like IList and ICollection have a lot of APIs that are required for those concrete types. Adding a method to an interface after it GAs is a breaking change so by calling an interface with only Read and Write operations IModel sort of means "cloud client models only read and write their content". Is there a possible future world where a cloud client model would have other behaviors we wanted to enforce via interfaces?

Copy link
Copy Markdown
Member

@annelo-msft annelo-msft Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other angle on this as a BCL type might be ... would any other non-model component ever want to use these behaviors as specified in the interface, and so we would want to name the interface to describe the behavior so that that would make sense?

If we want to stick with the "the interface describes the thing it is" approach because of the tight integration with other pieces in the content reader/writer API group, my spidey-sense is saying we might get asked to rename these something like ICloudServiceSchemaModel and ICloudServiceSchemaJsonModel -- we may want to consider whether we'd be happy with that outcome.

public readonly partial struct ModelReaderWriterFormat : IEquatable<ModelReaderWriterFormat>
{
internal const string JsonValue = "J";
internal const string WireValue = "W";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if there is a word that maps better to the BCL lexicon than Wire?

There is a nice article that maps out BCL terminology in this space that I spent some time with recently here: https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient It was very helpful in getting my head around how BCL things about HTTP network operations and what words they use to describe the various aspects of that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I know of do you have a suggestion?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not offhand, but I will think about it. Mostly just flagging that this could come up in the BCL review.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have an idea.

This is the one that is "whatever the service expects", is that correct?

If so, what about ServiceContract?

For me, "wire" communicates the transmission mechanism, e.g. "I send my message over the wire," and less about the piece that receives it on the other side of the wire. It feels a bit like a throwback to land-line telephone communications.

ServiceContract feels like it communicates better the intent of "what the service expects." It is the contract with the service. I believe BCL uses the word service in the same way, so this is at least consistent there, and if they have a better word for "contract," this name implies enough that they could proactively tell us that in a review, without having to ask us to explain what "wire" means first.

I also think it goes better alongside "Json" in the enum values, because a customer looking at those side-by-side might wonder "hey, the service contract using JSON, so this other Json must mean something else," and might be able to logic it out themselves, and understand it completely with a boost from good refdoc comments. Otherwise, it feels to me that people could get confused between whether to use "Wire" or "Json" when the service contract calls for JSON -- I know I was confused about that at first.

What do you think?

Copy link
Copy Markdown
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with the assumption that:

  • https://github.com/Azure/azure-sdk-for-net/pull/39421/files#r1367489853 will be addressed before merging.
  • If time allows, it would be great to address #39421 (comment) as well. For this one, I'm interested in understanding how disruptive this request is, and a bit concerned that either we'll forget to come back to it, or open the feature branch up to a larger set of contributors who'll copy the pattern of putting all tests in the internal test project and lose the benefits of making the split, and/or give ourselves a lot of work educating and chasing to get the outcome we want by setting the precedent early that the internal test project is only for tests of internal APIs.

I'm fine to come back to the naming comments in a later PR.

move some tests to public test project
public static Azure.Core.RequestContent Create(byte[] bytes) { throw null; }
public static Azure.Core.RequestContent Create(byte[] bytes, int index, int length) { throw null; }
public static Azure.Core.RequestContent Create(System.IO.Stream stream) { throw null; }
public static Azure.Core.RequestContent Create(System.Net.ClientModel.Core.IJsonModel<object> model, System.Net.ClientModel.ModelReaderWriterOptions? options = null) { throw null; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@m-nash m-nash merged commit b3a8f64 into feature/core-experiment Oct 20, 2023
@m-nash m-nash deleted the feature/modelReaderWriter branch October 20, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core OpenAI Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants