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

Model class design is inconvenient - make them POCOs #82

Open
douglasware opened this issue Jun 21, 2024 · 4 comments
Open

Model class design is inconvenient - make them POCOs #82

douglasware opened this issue Jun 21, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@douglasware
Copy link

I gave feedback on the serialization approach negatively impacting usability in the greater, .NET ecosystem. Here is some more:

Perhaps there is a design intention for the init only setters? Why can't I read an assistant creation definition I persisted and then modify these values? To me, it just makes the library hard to use. I don't see the upside. As it is, you might as well just have each of these properties as arguments on the create method which would certainly make the values immutable.

namespace OpenAI.Assistants
{
public partial class AssistantCreationOptions
{
internal IDictionary<string, BinaryData> _serializedAdditionalRawData;

    internal AssistantCreationOptions(string model, string name, string description, string instructions, IList<ToolDefinition> tools, ToolResources toolResources, IDictionary<string, string> metadata, float? temperature, float? nucleusSamplingFactor, AssistantResponseFormat responseFormat, IDictionary<string, BinaryData> serializedAdditionalRawData)
    {
        Model = model;
        Name = name;
        Description = description;
        Instructions = instructions;
        Tools = tools;
        ToolResources = toolResources;
        Metadata = metadata;
        Temperature = temperature;
        NucleusSamplingFactor = nucleusSamplingFactor;
        ResponseFormat = responseFormat;
        _serializedAdditionalRawData = serializedAdditionalRawData;
    }
    public string Name { get; init; }
    public string Description { get; init; }
    public string Instructions { get; init; }
    public IDictionary<string, string> Metadata { get; }
    public float? Temperature { get; init; }
}

}

Please simplify the implementation of all of the model classes and remove these arbitrary blockers.

@trrwilson trrwilson added the enhancement New feature or request label Jun 21, 2024
@trrwilson
Copy link
Collaborator

Thanks, @douglasware -- we've discussed this and we're going to make that change to have all top-level request option types like this be settable; #76 is another data point in that direction and we'll do it across the board.

As context: the overapplied motivation for this stems from client options, like specifying a non-default endpoint, and the confusion it can cause as to whether changing those persistent options affects the behavior of already-instantiated clients (it doesn't, but it's understandable that people would think it does). It's much less ambiguous that operations can't have their request options changed after the request has been sent; agreed that there's no good justification for making things harder in that case.

@janaka
Copy link

janaka commented Jun 26, 2024

Facing the same issue with ChatCompletionOptions. We are on C# 8 where init only setters are incompatible afaik. Think we can work about for now with reflection(tbc).

@dspear
Copy link

dspear commented Jun 26, 2024

@janaka I had problems with the init only setters, but I modified the project for the one module I have that's talking to this library to use C# 9, and then it was fine. I agree that it would be better for the library not to depend on C# 9 features, but there is this work-around.

@janaka
Copy link

janaka commented Jun 26, 2024

@dspear yeah, we might look at moving to C# 9 but it's part of a quite large system no idea where we might hit a rough edge and don't want to think about that right now :) Just going through making all the redlines go away in the lib directly depending on Azure OpenAI SDK. FWIW it's going ok.

Not run it yet but the following is my workaround for now.

internal ChatCompletionOptions TempInitOAIChatCompleteOptions((string, object)[] initProperties)
{
    //NOTE(v2 SDK): Workaround because c# 8 doesn't support init only setters. 
    // looks like they will change init only setters on the OpenAI lib 
    // see https://github.com/openai/openai-dotnet/issues/82
    var options = new ChatCompletionOptions();
    var optionsType = options.GetType();

    foreach (var (propertyName, propertyValue) in initProperties)
    {
        var property = optionsType.GetProperty(propertyName);
        if (property != null && property.CanWrite)
        {
            property.SetValue(options, propertyValue);
        } else 
        {
            throw new ArgumentException($"Error setting ChatCompletionOptions init property.Property {propertyName} does not exist or is read-only on {optionsType.FullName}.");
        }
    }
    return options;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants