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

Add http endpoints for sidecar functionality #359

Merged
merged 6 commits into from
Aug 17, 2023
Merged

Conversation

gjreda
Copy link
Collaborator

@gjreda gjreda commented Aug 16, 2023

part of #347

Running this PR

cd refstudio/python

poetry run uvicorn web:api --reload

You should then be able to visit http://127.0.0.1:8000/api/v0/docs in your browser and test various endpoints. Not all of them have been implemented yet, and some need some slight changes due to the way the current sidecar works (i.e. since we communicate via stdout, maybe of the existing sidecar functions do not return anything ... which needs to happen for the API).

Example usage

curl -s -X 'POST' \
  'http://127.0.0.1:8000/api/sidecar/ai/rewrite' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -d '{
  "text": "Chicago is the most populous city in the U.S. state of Illinois and the third-most populous in the United States after New York City and Los Angeles. With a population of 2,746,388 in the 2020 census, it is also the most populous city in the Midwest. As the seat of Cook County (the second-most populous U.S. county), the city is the center of the Chicago metropolitan area, one of the largest in the world.",
  "manner": "concise",
  "n_choices": 1,
  "temperature": 0.7
}' | jq

{
  "status": "ok",
  "message": "",
  "choices": [
    {
      "index": 0,
      "text": "Chicago is the most populous city in Illinois and the third-most populous in the United States, after New York City and Los Angeles. It has a population of 2,746,388 according to the 2020 census, making it the most populous city in the Midwest. It is also the center of the Chicago metropolitan area, which is one of the largest in the world and is located in Cook County, the second-most populous county in the US."
    }
  ]
}

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #359 (a216d67) into main (c1bebd9) will increase coverage by 0.33%.
Report is 2 commits behind head on main.
The diff coverage is 88.75%.

@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
+ Coverage   84.42%   84.75%   +0.33%     
==========================================
  Files         157      169      +12     
  Lines        9372     9684     +312     
  Branches     1022     1056      +34     
==========================================
+ Hits         7912     8208     +296     
- Misses       1449     1465      +16     
  Partials       11       11              
Files Changed Coverage Δ
python/main.py 0.00% <0.00%> (ø)
python/sidecar/ingest.py 89.70% <72.72%> (-0.67%) ⬇️
python/sidecar/http.py 88.63% <88.63%> (ø)
python/sidecar/chat.py 93.65% <100.00%> (+0.10%) ⬆️
python/sidecar/rewrite.py 94.20% <100.00%> (+0.17%) ⬆️
python/sidecar/search.py 51.35% <100.00%> (+2.77%) ⬆️
python/sidecar/storage.py 97.70% <100.00%> (+7.33%) ⬆️
python/web.py 100.00% <100.00%> (ø)

... and 21 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

danvk
danvk previously approved these changes Aug 16, 2023
Copy link
Collaborator

@danvk danvk left a comment

Choose a reason for hiding this comment

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

Looks good! Two high-level bits of feedback:

  1. It would be nice to have some tests
  2. Does this do validation of request bodies for us?

We'll want to get these endpoints into our codegen setup but that can be in a follow-up PR.

Also I'm seeing this error when I try to use the GET /ingest/references endpoint:

TypeError: Failed to execute 'fetch' on 'Window': Request with GET/HEAD method cannot have body.
image

python/sidecar/http.py Outdated Show resolved Hide resolved
python/sidecar/http.py Show resolved Hide resolved
python/sidecar/http.py Outdated Show resolved Hide resolved
python/main.py Outdated Show resolved Hide resolved
python/sidecar/http.py Show resolved Hide resolved
Copy link
Collaborator

@cguedes cguedes left a comment

Choose a reason for hiding this comment

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

Agree with Dan's comments.

We should have a distinct entry point for the HTTP server (from the cli/sidecar).

I see the backend for HTTP to translate http parameters (body, querystring) and call the internal functions that return a value. Then, that value is sent back to the client serialized as JSON.

For the CLI application interface, I see the app receiving requests in JSON (...Request) and also call the internal function that return a value. Then is the responsibility of the CLI to translate that to a sys.stdout.write(response.json()) reply.

The HTTP endpoints should also be adjusted to:

  • use the first segment to scope the different backend scopes:
    - references (ingestion, status, edit, delete, ...),
    - ai (completion, chat)
    - search (s2)
  • adopt the best HTTP method
    - GET to access a resource or query information
    - I think we should use this method for text completion and for s2 search)
    - DELETE to delete a resource (no need to repeat delete in the URL, we should have an URL that identify a resource)
    - PATCH to partially update a reference

Alternatively, if we want to mimic the sidecar request/reply method of sending a JSON payload and receiving a JSON payload, we should only use POST to a api/v0/sidecar/... endpoint.


We also need to discuss how the settings (ex: OPENAI_API_KEY) are sent/accessible in the HTTP server. We know that the setting is configured by the client.

@gjreda
Copy link
Collaborator Author

gjreda commented Aug 17, 2023

For the CLI application interface, I see the app receiving requests in JSON (...Request) and also call the internal function that return a value. Then is the responsibility of the CLI to translate that to a sys.stdout.write(response.json()) reply.

@cguedes I agree with this approach, though it's a fairly sizable refactor (mainly the tests) that I think would be distracting in this PR. I'll add it as a follow up.

danvk
danvk previously approved these changes Aug 17, 2023
@@ -1,7 +1,7 @@
import inspect
import json

from sidecar import chat, cli, ingest, rewrite, storage, search
from sidecar import chat, cli, ingest, rewrite, search, storage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessary for this PR, but is there a linter/formatter that's not running as part of CI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, this is something my vs-code only does for refstudio and I haven't been able to figure out why

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had thought it might have been due to prettier integration, but seems not

@gjreda gjreda marked this pull request as ready for review August 17, 2023 20:18
danvk
danvk previously approved these changes Aug 17, 2023
@cguedes cguedes merged commit eb78d8c into main Aug 17, 2023
11 checks passed
@cguedes cguedes deleted the http-endpoints-fastapi branch August 17, 2023 20:36
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

3 participants