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

[Refactor] Unify endpoints response / REST API v2 #764

Open
valentimarco opened this issue Mar 28, 2024 · 9 comments
Open

[Refactor] Unify endpoints response / REST API v2 #764

valentimarco opened this issue Mar 28, 2024 · 9 comments
Assignees
Labels
documentation Improvements or additions to documentation endpoints Related to http / ws endpoints enhancement New feature or request good first issue Good for newcomers V2

Comments

@valentimarco
Copy link
Collaborator

valentimarco commented Mar 28, 2024

At the moment all the endpoints response are not uniform ( sometimes we give jsonSchema, sometimes a json with some field etc) and can create problem when the user uses the REST API instead of Language Specific clients, like the Typescript client.

I suggest the use of JSend as standard JSON response for all endpoints and change all HTTPExceptions responses, FastAPI provide the override of these exceptions with a simple function.

Fast API is a typed Backend framework, so I suggest also the use of an object call JSendResponse that wraps the JSend standard.

ALL of this must be under this versioned prefix /api/v2 so we can delay the breaking changes!

P.S
JSend is usually the simplest and have many variations (only have status and data where wraps a string or an object, status can be the HTTP status code instead of a string etc…) !

@valentimarco valentimarco added documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers endpoints Related to http / ws endpoints labels Mar 28, 2024
@valentimarco
Copy link
Collaborator Author

@enrichman Thx for the versioned prefix suggestion!

@pieroit
Copy link
Member

pieroit commented Apr 20, 2024

  • full CRUD over documents and chunks
  • user_id on each endpoint
  • authentication???

@pieroit pieroit changed the title [Refactor] Unify endpoints response [Refactor] Unify endpoints response / REST API v2 Apr 20, 2024
@pieroit pieroit moved this from 📋 Backlog to 🏗 In progress in Cat Project Kanban Apr 20, 2024
@pieroit
Copy link
Member

pieroit commented Apr 29, 2024

(Notes from dev meeting)

V2 REST API:

  • DEVELOPMENT

    • namespace under /v2/...
    • deprecation warning on v1 endpoints
    • delete v1 endpoints at Cat v2 release
    • migrate tests directly to v2
  • NAMING STANDARD

    • NO SLASH con redirect verso SLASH
    • dash notation / da vedere con le classi
    • setting vs settings ???
    • better semantic ordering (/plugins/nome-plugin/settings)
  • ENDPOINT HIERARCHY

    • Settings (TO BE DELETED)
    • LLM
    • Embedder
    • Plugin
    • Memory
      • only episodic and declarative
      • resources are collection and point
      • POST chunk + metadata
    • Conversation
      • GET / DELETE / PUT convo history
    • Rabbithole
      • no edits
  • MEMORY CRUD (only collection, point and metadata)

    • only identity filters! {"source": "my.pdf"}
      (no: <, >, !=, in [], contains)
  • ENDPOINT CUSTOM

    • (hacky) manual input
    • (top) hook:
      • we pass cheshire_cat_api as router, and you can use include_router
      • we pass the router obj already namespaced under /custom
    • (complex) decorator + add_api_route done by mad_hatter
  • HTTP STREAMING

    • stays hanging / experimental

@dave90
Copy link
Contributor

dave90 commented Apr 30, 2024

I would like to work on the issue :)
Before proceeding, I have a proposal regarding the implementation that I'd like to get feedback on.

My idea is to create 2 APIRouter and for each endpoint function create 2 functions (one for each version). The differences is that in the v2 endpoint function wraps the result into a JSendResponse. For example:

class JSendResponse(BaseModel):
    status:JSendStatusEnum
    data: Union[Dict, None] = None
    message:Union[str, None] = None
# Default router
router_v1 = APIRouter()

# Router v2
router_v2 = APIRouter()

def home() -> Dict:
    with open("pyproject.toml", "rb") as f:
        project_toml = tomli.load(f)["project"]

    return {
        "status": "We're all mad here, dear!",
        "version": project_toml['version']
    }


# server status
@router_v1.get("/")
async def home_v1() -> Dict:
    """Server status""" 

    return home()

@router_v2.get("/", response_model=JSendResponse, response_model_exclude_none=True)
async def home_v2():
    """Server status""" 
    
    return {
        "status" : JSendStatusEnum.success,
        "data" : home()
    }

And then in main.py add the endpoints for v2.

This implementation keeps the behaviour of the current endpoints but requires the refactoring of all the endpoint functions. What do you think? I am open to suggestions if you think there is a better way to implement this.

@pieroit
Copy link
Member

pieroit commented Apr 30, 2024

@dave90 agree on the refactoring strategy, I don't know about the JSON send because from the research I'm doing it is not widespread and only adds overhad on directly inspecting the status code of the http response.

Also many are suggesting to have Pydantic types for all endpoints, I don't see the point of encapsulating them inside {"data": {}}

I assigned this issue to you, start from something easy and do small PRs ;)
Refactoring the endpoints one route at a time is a good idea, so we can test properly
I'm available for help and support anytime

Thanks!!!

@valentimarco
Copy link
Collaborator Author

Agrre with @pieroit, Data should be a Union of Classes (LLMSettings,EmbedderSettings etc... )!

@pieroit
Copy link
Member

pieroit commented Apr 30, 2024

Agrre with @pieroit, Data should be a Union of Classes (LLMSettings,EmbedderSettings etc... )!

My points are:

  • the standard you guys mention does not seem supported and well spread
  • there is no point in repeating the status code
  • data as a key is not needed, it just makes the json deeper

@valentimarco
Copy link
Collaborator Author

Agrre with @pieroit, Data should be a Union of Classes (LLMSettings,EmbedderSettings etc... )!

My points are:

* the standard you guys mention does not seem supported and well spread

* there is no point in repeating the status code

* data as a key is not needed, it just makes the json deeper
  1. Jsend is the easiest one and most REST responses are similar or follow the same logic of Jsend(See this). We can change it to this but is very complex and unnecessary.
  2. Jsend don't require the status code in the body, the status param is a enum of 3 strings: success, error,fail.
  3. Data key contains the data of the response. For the user prospective is much easier, He knows that each time the response is 2xx the Data key is where i can find the data (in case of LLM models, i will get the Json Schema)

@valentimarco
Copy link
Collaborator Author

(Notes from dev meeting)

V2 REST API:

* DEVELOPMENT
  
  * namespace under `/v2/...`
  * deprecation warning on v1 endpoints
  * delete v1 endpoints at Cat v2 release
  * migrate tests directly to v2

* NAMING STANDARD
  
  * NO SLASH con redirect verso SLASH
  * dash notation / da vedere con le classi
  * setting vs settings ???
  * better semantic ordering (/plugins/nome-plugin/settings)

* ENDPOINT HIERARCHY
  
  * Settings (TO BE DELETED)
  * LLM
  * Embedder
  * Plugin
  * Memory
    
    * only episodic and declarative
    * resources are collection and point
    * POST chunk + metadata
  * Conversation
    
    * GET / DELETE / PUT convo history
  * Rabbithole
    
    * no edits

* MEMORY CRUD (only collection, point and metadata)
  
  * only identity filters! `{"source": "my.pdf"}`
    (no: <, >, !=, in [], contains)

* ENDPOINT CUSTOM
  
  * (hacky) manual input
  * (top) hook:
    
    * we pass cheshire_cat_api as router, and you can use include_router
    * we pass the router obj already namespaced under `/custom`
  * (complex) decorator + add_api_route done by mad_hatter

* HTTP STREAMING
  
  * stays hanging / experimental
  • Rabbithole
    • Endpoints must accept a json rappresenting the metadata

@pieroit pieroit added the V2 label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation endpoints Related to http / ws endpoints enhancement New feature or request good first issue Good for newcomers V2
Projects
Status: 🏗 In progress
Development

No branches or pull requests

3 participants