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

realtime specific entry point + Java emitter config #325

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jpalvarezl
Copy link
Collaborator

@jpalvarezl jpalvarezl commented Nov 14, 2024

What's this?

A custom entry point to allow code gen for realtime specifically by having a tsp-location.yaml that looks like this:

directory: .typespec.entry_points/realtime
commit: <commit-id>
repo: joseharriaga/openai-in-typespec
additionalDirectories:
  - .typespec/realtime

Learnings

While testing how to best approach this, I have learn the following about the tsp-client behaviour, which is what I understand is the recommended way of generating code in the SDK repositories. (I will be using realtime as the example)

  • Since we have directory: .typespec.entry_points/realtime as a target folder, when running tsp-client sync to populate the TempTypeSpecFiles folder, we will only get the contents of the folder itself, but not the actual TSP definition found in the ../.typespec directory. (This might be obvious enough, even though we are importing other files, I don't think that the tooling has a concept of a project tree to build)
  • The next step would then be to add additionalDirectories: .typespec/realtime (just to scope the pulling of files to what we strictly need). The result of this step was unexpected. Files under directory and additionalDirectories are flattened under TempTypeSpecFiles which breaks any folder structure in the source repository.
  • The previous step behaviour results in the import statements in the .typespec.entry_point/*/main.tsp files to be completely meaningless, since these will be flattened with anything coming in from the additionalDirectories.
  • The reason I placed .typespec.entry_points at the same level of .azure.typespec and .typespec is because we will (do) have entry points for both folders, so hierarchically speaking, it makes sense.

How to move forward

There are a few possibilities:

Approach 1: feature requesting the tooling to change behaviour

  • We dig deeper on the flattening of directory and additionalDirectories behaviour, try to find out why it is the way it is and whether this is behaving according to some agreed spec that would be hard to change. This approach in all likelihood won't get us very far, that behaviour is probably by now, a well established assumption taken into account by many developers at this point. Pros and cons:
    • 🟢 This approach could render the tooling more intuitive and well adjusted to our use case
    • 🔴 High friction. Lots of communication and agreements need to happen before this can move forward. Maybe the way I am expecting this tooling to work isn't even the "intuitive way" for other teams.

Approach 2: root folder entry points + jive with folder flattening

  • We move forward assuming the folder flattening happens and we go along:
    • Under .typespec/realtime I removed client.tsp.
    • Under .typespec.entry_points/realtime I have added a client.tsp
    • Adjusted the import statement in .typespec.entry_points/realtime/client.tsp to assume that main.tsp is in the same folder
    • Moving forward, under "feature group" folders the client.tsp filename has become vetoed and a reserved name for bootstraping entry points to services.
  • Pros and cons:
    • 🟢 we can move forward independently of any tooling adjustments
    • 🟢 this works today
    • 🔴 The flattening happens when fetching the TSP definition into SDK repositories. What is the behaviour from the CI perspective? The folder structure might just be preserved, which will render our client.tsp files import statements invalid.

Approach 3: Avoid folder flattening behaviour + duplication

  • One additional approach I foresee, is to move the .typespec.entry_points into .typespec and .azure.typespec
  • Pros and cons:
    • 🟢 same as the previous approach
    • 🟢 Would work well both while fetched from an SDK repo or the CI as the import structure would be in sync with the project tree (no flattening)
    • 🔴 This would require duplication in the .azure.typespec and .typespec scenarios.

Remaining TODOs

  • Confirm that the other emitters using this spec behave the same
  • Better extraction of what was originally in .typespec/realtime/client.tsp. Now it's all dumped in its corresponding main.tsp file.

@jpalvarezl jpalvarezl requested a review from trrwilson November 14, 2024 17:54
@jpalvarezl
Copy link
Collaborator Author

@trrwilson , it would be great to be able to merge this if we are able to move forward with the dev feed release of the Java client library (Azure/azure-sdk-for-java#42707). Let me know if this needs updating in anyway so that it works well with the other emitters being used. Thank you!

})
@server("wss://api.openai.com/v1", "OpenAI Endpoint")
@useAuth(BearerAuth)
namespace OpenAI;
Copy link
Owner

Choose a reason for hiding this comment

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

I think converting this client.tsp into a main.tsp is a little confusing.

Would it work if we did the following?

  1. Create a separate folder under the ".typespec" folder called ".java", where we put this file (but call it "main.tsp") and also the "tspconfig.yaml" file.
  2. Rename the "dotnet.tsp" and "java.tsp" files to "client.dotnet.tsp" and "client.java.tsp".
  3. Move the "client.dotnet.tsp" and "client.java.tsp" files under ".typespec/realtime" instead of ".typespec/realtime/custom".
  4. Revert the changes to the "/.typespec/realtime/main.tsp", but edit it to import both the "client.dotnet.tsp" file and "client.java.tsp" file.
  5. Revert the changes to the "custom.tsp" file, and move it under the "custom" folder with the rest of the custom models.

Copy link
Collaborator Author

@jpalvarezl jpalvarezl Dec 12, 2024

Choose a reason for hiding this comment

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

Thanks for all the feedback! Agreed in all accounts. Just regarding point 1 and after discussing with @trrwilson, I think that instead of having a cut "per language" we might actually prefer to have a cut "per artifact" we are hoping to generate from the spec.

I will go ahead and create a separate folder where we can start defining entry points as we need them. For now, we generally need:

  • Catch all entry point (for .NET and OpenAPIv3)
  • Realtime specific (Java)
  • Other entry points we might need in the future, but is highly dependent on how we move forward with the companion libraries to the stainless ones: batch, inference, assistants, etc.

Let me know what you think about this approach.

Copy link
Collaborator Author

@jpalvarezl jpalvarezl Dec 12, 2024

Choose a reason for hiding this comment

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

Regarding moving custom.tsp, I didn't really do anything other than adding a couple of imports and removing an unused import which was breaking code gen in the java repo. Would it be okay to omit this change in this PR, as I would like to focus on the multiple entry point part of it and the Java support for realtime instead?
More than happy to follow up in a different PR with any restructuring that may be needed.

@jpalvarezl jpalvarezl force-pushed the jpalvarezl/realtime_entry_point branch from 8b35c3f to c26e90a Compare December 12, 2024 02:49
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.

2 participants