Skip to content

UN-2854 - Initial swagger.json and optional CORS headers #111

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

Merged
merged 9 commits into from
Mar 25, 2025

Conversation

d1donlydfink
Copy link
Collaborator

@d1donlydfink d1donlydfink commented Mar 24, 2025

Get enough of a manual swagger.json going so that swagger-ui container can largely do what it needs to.
@andreidenissov-cog reports that Postman largely does better with the streaming.

Will eventually need to push a new release for this for the external-facing server.

@@ -0,0 +1,429 @@
{
"openapi": "3.0.3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Manual edit of an OpenAPI v3.0 file that @andreidenissov-cog got from gnostic (?)
Function and Connectivity worked fine, but I had to then manually tweak it and the server to get StreamingChat
working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow that's very useful. We should probably make it available in the neuro-san-demos docs / tutorials at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The next step is to be able to generate this automatically when things change.

@@ -37,7 +37,7 @@ async def stream_out(self,
:param generator: async gRPC generator
"""
# Set up headers for chunked response
self.set_header("Content-Type", "application/json")
self.set_header("Content-Type", "application/json-lines")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OAI/OpenAPI-Specification#3730

Says that json-seq is not yet supported until OpenAPI 3.2, but another issue mentioned that json-lines was supported. Close enough.

"""
Implementation of OPTIONS request handler for streaming chat API call.
"""
await self.flush()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Handle OPTIONS requests for streaming_chat. This allows for swagger-ui container to work w/ CORS fixes.
No idea why. 🤷

if os.environ.get("AGENT_ALLOW_CORS_HEADERS") is not None:
self.set_header("Access-Control-Allow-Origin", "*")
self.set_header("Access-Control-Allow-Methods", "GET, POST, OPTIONS")
self.set_header("Access-Control-Allow-Headers", "Content-Type, Transfer-Encoding")
Copy link
Collaborator Author

@d1donlydfink d1donlydfink Mar 25, 2025

Choose a reason for hiding this comment

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

Put the CORS headers behind an env var that is not on by default.
@sidcog is going to configure these for the externally facing server on the load balancer.
For localhost to work with swagger-ui container though, these need to come from inside the house.

According to all the docs, this "*" is definitely not the right thing to do.
That said, I don't know what the right thing to do is just yet.

Be sure OPTIONS is part of that number as well.

@d1donlydfink d1donlydfink changed the title Progress towards using an OpenAPI 3.0 description using swagger-ui co… UN-2854 - Initial swagger.json and optional CORS headers Mar 25, 2025
Copy link
Contributor

@ofrancon ofrancon left a comment

Choose a reason for hiding this comment

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

lgtm. Should make it a lot easier for people to use the http service.

@@ -0,0 +1,429 @@
{
"openapi": "3.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow that's very useful. We should probably make it available in the neuro-san-demos docs / tutorials at some point.

@d1donlydfink d1donlydfink requested a review from deepsaia March 25, 2025 15:36
@d1donlydfink d1donlydfink merged commit 50f46b4 into main Mar 25, 2025
3 checks passed
@d1donlydfink d1donlydfink deleted the DEF-open-api-swagger-ui branch March 25, 2025 15:37
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