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

Overlay support spec doc #1598

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from
Draft

Conversation

njaci1
Copy link
Contributor

@njaci1 njaci1 commented Mar 18, 2024

add a folder for specs and add the first spec doc for overlay support.

Support use of Overlays for enabling developers to enhance existing OpenAPI description files without changing the original file.

## Problem Statement
Existing OpenAPI documents used for AI plugin creation might lack necessary properties or require modifications for them to provide a high quality AI plugin. Direct editing of the original OpenAPI document is often undesirable or impractical.

Choose a reason for hiding this comment

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

Not AI plugin, but I'd even say referenced OpenAPI descriptions that participate in the GPTs and Plugins ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you have phrased it better. I'll rephrase in the spec to capture this.

Choose a reason for hiding this comment

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

Why restrict the use-case of overlays to AI plug-ins when OpenAPI.NET library caters to a diverse range of users?

Choose a reason for hiding this comment

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

@adhiambovivian has a good point. What sparked this discussion was definitely AI plugins. But it's absolutely true that overlays are more than just for AI plugins, we see it everyday in Kiota. Let's frame it as a more generic scenario and support it with real world examples, like AI plugins, client code generation, etc.

Choose a reason for hiding this comment

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

And for the copilot use-cases, could you please provide more specific details about the copilot use-cases?

Copy link

@maisarissi maisarissi Mar 26, 2024

Choose a reason for hiding this comment

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

Sure @adhiambovivian . For copilot scenarios we might would want to provide instructions to the orchestrator on how to use the certain function (/createSong as an exmaple), add localization, provide information on how to present the response to the user (like using adaptive cards or other rendering) etc. Having the possibility to augment OpenAPI documents with overlays and provide more information to AI orchestratos/Copilot will unclock several scenarios.

description: Successful response
```

**Overlay File (overlay.yaml)**

Choose a reason for hiding this comment

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

This overlay doesn't use JSON Path to define the structure. Are we saying that this is our MVP for now? Only structured overlays vs. targeted overlays would be supported?

https://github.com/OAI/Overlay-Specification/blob/main/versions/1.0.0.md#targeted-overlays

I'm honestly OK at this point for either, but make it clear would be better and provide a more appropriate debate!

Copy link
Member

Choose a reason for hiding this comment

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

You can't really separate the two. Targeted overlays use a merge patch at the target. Structured overlays are just the trivial case of targeted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebastienlevert is your question whether we will support either structured or targeted rather than both? We will support both cases.

@darrelmiller
Copy link
Member

We should add some discussion of the developer experience of applying an overlay. Is this a separate OverlayService class? Is it an extension method on OpenAPIDocument? Does it change the existing document or create a new patched document? Is it in a different project? If not, how do we handle the YAML dependency?

@@ -0,0 +1,106 @@
# Feature Specification: Overlays Support

## Objective

Choose a reason for hiding this comment

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

Shouldn't the spec include customer application and target audience?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have revised the objective statement to try capture the target audience.

@MaggieKimani1
Copy link
Contributor

Maybe use this issue to add onto some of the use cases or scenarios where overlays would come in handy #615

expand the problem statement to capture more areas of challenges.
@njaci1
Copy link
Contributor Author

njaci1 commented Mar 27, 2024

We should add some discussion of the developer experience of applying an overlay. Is this a separate OverlayService class? Is it an extension method on OpenAPIDocument? Does it change the existing document or create a new patched document? Is it in a different project? If not, how do we handle the YAML dependency?

@darrelmiller I have added a section on 'User Journey' to start the discussion. Have a look at it and continue the discussion.

revised with reviewers' comments
3. Use the OverlayService to apply the overlay file to the OpenAPI document. The OverlayService parses the overlay file, applies the changes directly to the OpenApiDocument instance, and performs in-place modifications.
```csharp
var overlayService = new OverlayService();
overlayService.ApplyOverlay(openApiDocument, overlayPath);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
overlayService.ApplyOverlay(openApiDocument, overlayPath);
var overlay = await Overlay.LoadAsync(stream);
var jsonElement = await JsonDocument.ParseAsync(stream).RootElement;
var newJsonElement = overlay.Apply(jsonElement);

Copy link
Member

Choose a reason for hiding this comment

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

End to end process would be:

  • Use OpenApiDocument.Load to load JSON/Yaml file with Overlay referenced in readerSettings
  • Load Yaml and translate to JSONNodes or just load JSONNodes
  • Load Overlay as JsonNodes.
  • Apply Overlay to OpenAPI JsonNodes
  • Pass "overlayed" JsonNodes to rest of OpenAPI Parser.

address reviewer comments. Add example for targeted overlay file.
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.

6 participants