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

Adds function call Interface and response. #70

Merged
merged 11 commits into from
Jun 16, 2023
Merged

Conversation

alpha-se
Copy link
Contributor

@alpha-se alpha-se commented Jun 16, 2023

Adds function call interface including some type-safeish definition interface

Copy link
Contributor

@davidmigloz davidmigloz left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I was planning to issue a similar PR.

Looks good to me. I've added some basic documentation suggestions (although some more would be required to keep the project's standards).

There are some linter warnings that may be good to fix. And maybe adding some tests would be good too.

lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Outdated Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/core/models/functions/functions.dart Show resolved Hide resolved
lib/src/instance/chat/chat.dart Outdated Show resolved Hide resolved
lib/src/instance/chat/chat.dart Outdated Show resolved Hide resolved
@davidmigloz
Copy link
Contributor

I was actually wondering whether it is worth using the strong type OpenAiFunctionParameters for parameters or just using a Map<String, dynamic> would be better.

I guess many people will have their JSON Schema already defined as a JSON (e.g. if using OpenAPI), or you could easily generate it using something like JSON to JSON Schema Converter (or asking ChatGPT).

If parameters would be defined as Map<String, dynamic>, you could just copy-paste your JSON Schema. Otherwise, you have to convert it to OpenAiFunctionParameters.

Co-authored-by: David Miguel Lozano <davidmigloz@users.noreply.github.com>
@alpha-se
Copy link
Contributor Author

My reasoning here is that the OpenAI docs use a (currently only found a single value) type parameter "object".
My assumption is that there is some way of passing a singular parameter differently (e.g. only doing type: String and passing the parameter field with the value directly.

By wrapping this in a type we can account for that.
Not sure it makes sense to do so right away until there's more clarity around how the API on the OpenAI end actually looks but maybe a different factory constructor for OpenAiFunctionModel that takes the parameters as a map directly makes more sense? I'd prefer to get this in though and unblock people and then add sugar syntax later, wdyt?

@anasfik
Copy link
Owner

anasfik commented Jun 16, 2023

Thank you so much for your work, this will be reviewed right now.

@davidmigloz
Copy link
Contributor

davidmigloz commented Jun 16, 2023

My reasoning here is that the OpenAI docs use a (currently only found a single value) type parameter "object".
My assumption is that there is some way of passing a singular parameter differently (e.g. only doing type: String and passing the parameter field with the value directly.

The docs say "The parameters the functions accepts, described as a JSON Schema object". So, if you want to describe a single string, you would have to pass:

{
  "type": "string"
}

Another advantage of using as Map<String, dynamic> is that you could use a package like json_schema to validate it.

@alpha-se
Copy link
Contributor Author

The docs say "The parameters the functions accepts, described as a JSON Schema object". So, if you want to describe a single string, you would have to pass:

Yeah, but does that actually ever make sense? Since OpenAI expects an object that defines a subschema here, it's unclear to me that there is ever an advantage to using a map directly. (As opposed to having typesafety for the required fields etc.
I hear your point though, I think we could allow for just passing parameters as a Map<String, dynamic> and have the OpenAiFunctionParameters class be a stand-in for having a bit more syntactic sugar around creating that, e.g.

parameters: OpenAiFunctionParameters(...).toMap(),

WDYT?

@davidmigloz
Copy link
Contributor

davidmigloz commented Jun 16, 2023

What about having OpenAiFunctionParameters as a sealed class like:

sealed class OpenAiFunctionParameters {
  const OpenAiFunctionParameters();

  factory OpenAiFunctionParameters.fromMap(
    final Map<String, dynamic> parameters,
  ) {
    return OpenAiFunctionParametersMap(parameters);
  }

  factory OpenAiFunctionParameters.typed(
    final String type,
    final List<OpenAiFunctionProperty> properties,
  ) {
    return OpenAiFunctionParametersTyped(
      type: type,
      properties: properties,
    );
  }

  Map<String, dynamic> toMap();
}

class OpenAiFunctionParametersMap extends OpenAiFunctionParameters {
  final Map<String, dynamic> parameters;

  const OpenAiFunctionParametersMap(this.parameters);

  @override
  Map<String, dynamic> toMap() => parameters;
}

class OpenAiFunctionParametersTyped extends OpenAiFunctionParameters {
  final String type;
  final List<OpenAiFunctionProperty> properties;

  const OpenAiFunctionParametersTyped({
    this.type = 'object',
    required this.properties,
  });

  @override
  Map<String, dynamic> toMap() {
    final requiredProperties = properties
        .where((property) => property.isRequired)
        .map((property) => property.name)
        .toList();
    return {
      'type': type,
      'properties': {
        for (final parameter in properties)
          parameter.name: {
            if (parameter.type != null) 'type': parameter.type,
            if (parameter.description != null)
              'description': parameter.description,
            if (parameter.enumValues != null) 'enum': parameter.enumValues,
          },
      },
      'required': requiredProperties,
    };
  }
}

In that way, we still keep parameters as an object we control (so that we can account for future cases) and we support both use cases, dynamic map, and type-safety.

@alpha-se
Copy link
Contributor Author

Hm, seems like a bit of overkill to make it sealed since we're just wrapping an HTTP API.

I think we can go with something easier like

class OpenAiFunctionParameters {
  final Map<String, dynamic> _map;
  const OpenAiFunctionParameters(Map<String, dynamic> parametersSchema) : _map = parametersSchema;

  factory OpenAiFunctionParameters.forProperties(
    final List<OpenAiFunctionProperty> properties,
  ) {
    final requiredProperties = properties
        .where((property) => property.isRequired)
        .map((property) => property.name)
        .toList();
    return OpenAiFunctionParameters(
      <String, dynamic>{
        'type': 'object',
        'properties': {
        for (final parameter in properties)
          parameter.name: parameter.toMap(),
         'required': requiredProperties,
      };
  }

  Map<String, dynamic> toMap() => _map;
}

@@ -51,6 +63,7 @@ final class OpenAIChatCompletionChoiceMessageModel {

return other is OpenAIChatCompletionChoiceMessageModel &&
other.role == role &&
other.content == content;
other.content == content &&
other.functionCall == functionCall;
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this will have no effect currently, since the FunctionCallResponse does not override the == operator.

}

class OpenAiFunctionProperty {
static const functionTypeInteger = 'integer';
Copy link
Owner

Choose a reason for hiding this comment

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

So we follow a convention in the package, can we set this type to an enum in the enum.dart file, I think it would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, they are not really enums is the problem. I generally want the user to send whatever string type they want to the API, these are just helper methods to give a bit more syntactic sugar

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good, but with this, the user might think that those are the 4 only accepted types that he can pass to the request, don't you agree?

README.md Outdated
```dart
FunctionCallResponse? response = chatCompletion.choices.first.message.functionCall;
String? functionName = response?.name;
Map<String, dynamic>? functionParameters = response.parameters;
Copy link
Owner

Choose a reason for hiding this comment

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

I did write some examples in the example/ folder locally following this, which would be helpful.

@anasfik anasfik merged commit 36d14bd into anasfik:main Jun 16, 2023
@davidmigloz
Copy link
Contributor

Thanks for your work!

Looking forward to integrating it to https://github.com/davidmigloz/langchain_dart 🙂

@davidmigloz
Copy link
Contributor

I've issued a new PR with some missing pieces for having a working implementation:

#71

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

Successfully merging this pull request may close these issues.

None yet

6 participants