canopy server documentation #136

miararoy · 2023-11-02T08:13:17Z

Problem

FastAPI server needs to be documented

Solution

document the api methods
add response code documentation
document the API models
add fastapi app description
add CLI command canopy api-docs

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Infrastructure change (CI configs, etc)
Non-code change (docs, etc)
None of the above: (explain here)

Test Plan

Documentation linting and regression

igiloh-pinecone

There some things that need to be changed, namely:

The mechanism for generating docs (won't work in a PyPI package).
The new ContextContentResponse type
Incorrect documentation of some of the API parameters which are in fact not supported

src/canopy/models/data_models.py

igiloh-pinecone · 2023-11-02T08:45:30Z

src/canopy_cli/__init__.py

This file definitely doesn't belong in the canopy_cli directory (package).

Usually, I would put this file somewhere under the repo's root (not inside src) and use it only in CI. But I get it that you want users to be able to build the documentation locally.
In that case - I'm not even sure this file will be included in the .whl without explicitly being included in the pyproject.toml.
Please check what is the best practice for allowing user's to get documentation locally.

igiloh-pinecone · 2023-11-02T08:50:33Z

src/canopy_cli/cli.py

+        """
+    )
+)
+def docs():


Again - I'm not sure that's a conventional practice...

Can't you just do something like:

check_service_health() webbrowser.open('http://localhost:8000/rdocs')

It's so much simpler and more elegant...

src/canopy_server/app.py

nc2112

Few small changes

src/canopy/models/data_models.py

nc2112 · 2023-11-02T15:31:47Z

src/canopy_server/__init__.py

+## Prerequisites
+
+### Pinecone API key
+To get Pinecone free trial API key and environment register or log into your Pinecone account in the console (https://app.pinecone.io/). You can access your API key from the "API Keys" section in the sidebar of your dashboard, and find the environment name next to it.


We should say 'free' rather than 'free trial' for Pinecone. Trial implies it ends, which our free tier does not

byronnlandry

Made various suggestions for usage, correctness, and concision.

src/canopy/models/data_models.py

byronnlandry · 2023-11-02T16:26:16Z

src/canopy/models/data_models.py

+    )
+    metadata: Metadata = Field(
+        default_factory=dict,
+        description="The document metadata, to learn more about metadata, see https://docs.pinecone.io/docs/manage-data",  # noqa: E501


Suggested change

description="The document metadata, to learn more about metadata, see https://docs.pinecone.io/docs/manage-data", # noqa: E501

description="The document metadata. To learn more about metadata, see https://docs.pinecone.io/docs/manage-data", # noqa: E501

byronnlandry · 2023-11-02T16:26:41Z

src/canopy/models/data_models.py

@@ -69,12 +88,12 @@ class Role(Enum):


 class MessageBase(BaseModel):
-    role: Role
-    content: str
+    role: Role = Field(description="The role of the messages author.")


Suggested change

role: Role = Field(description="The role of the messages author.")

role: Role = Field(description="The role of the message author.")

Suggested change

role: Role = Field(description="The role of the messages author.")

role: Role = Field(description="The role of the message's author.")

src/canopy_server/__init__.py

src/canopy_server/app.py

Co-authored-by: byronnlandry <[email protected]>

Instead of using process._parent_id which is not garuranteed, use `os.getppid()`

This is the official method by the mp module, which should work across all OSes (hopefully..)

It shouldn't theotically happen, but who knows...

This was a horrible name that doesn't represent the true meaning of this class

Per Nathan and Byron's feedback

Co-authored-by: byronnlandry <[email protected]>

This way they are only generated if the user want to generate them locally

Much more readable

igiloh-pinecone

Fixed all issues. Ready for merge

This way, running 'canopy --help' prints the errors in the order matching the quick start

This conforms with the naming we use in the documentation

Done

In order to support our current StuffingContextBuilder, I added a new StuffingConxtextContent which inherits from ContextContent and implement to_text() correctly. The app's `/query` path returns a `str`, which is the only guaranteed format of Context. It can be any strucured on unstrucutured data - depending on the ContextBuilder

- Made StuffingContextContent always a List - Slightly improved readability of `StuffingContextBuilder`

I changed the tests to use explicit json.loads()

Minor CLI improvements

review-notebook-app · 2023-11-06T09:14:03Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Makes the code more readable and explicit

KISS solution - simply return a different model than the actual internal `Context`

Context must contain a ContextContent that implements to_text()

miararoy added 4 commits November 2, 2023 10:10

canopy server documentation

7ed9291

fix response type in Chat

bdea5a6

Merge branch 'dev' into server-docstrings

78e2238

ignore line mypy

001bac8

igiloh-pinecone suggested changes Nov 2, 2023

View reviewed changes

miararoy added 7 commits November 2, 2023 12:45

remove soon dep. flag

78a0a3b

Add 'ignored' for model param

2ad5385

stream descp

575301c

user descp

257130c

batch size

553661c

app.py

d4a725e

fix trailing whitespace

c7a014c

nc2112 suggested changes Nov 2, 2023

View reviewed changes

byronnlandry previously requested changes Nov 2, 2023

View reviewed changes

miararoy and others added 10 commits November 2, 2023 22:23

fix

1d9d566

Apply suggestions from code review

acc42c3

Co-authored-by: byronnlandry <[email protected]>

[app] Don't use private attribute

8227d84

Instead of using process._parent_id which is not garuranteed, use `os.getppid()`

[app] Parent process - use an even better solution

6f4b0a1

This is the official method by the mp module, which should work across all OSes (hopefully..)

[app] Properly handle None case

9db8c0f

It shouldn't theotically happen, but who knows...

[models] Rename ContextContentResponse

8f019b6

This was a horrible name that doesn't represent the true meaning of this class

[app] Fix description

08a7f23

Per Nathan and Byron's feedback

Apply Byron's and Nathan's suggestions from code review

3d16600

Co-authored-by: byronnlandry <[email protected]>

[app] Made generating the docs optional

2bbba59

This way they are only generated if the user want to generate them locally

[app] Moved docs template to their own dedicated file

4ac9ff9

Much more readable

igiloh-pinecone approved these changes Nov 5, 2023

View reviewed changes

fix linter issues

71c7fc6

igiloh-pinecone enabled auto-merge November 5, 2023 10:42

igiloh-pinecone added 2 commits November 5, 2023 13:09

[CLI] Control commands order in help message

3e63f95

This way, running 'canopy --help' prints the errors in the order matching the quick start

[cli] Rename 'service' to 'server'

3f8acd2

This conforms with the naming we use in the documentation

igiloh-pinecone added this pull request to the merge queue Nov 5, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 5, 2023

igiloh-pinecone added this pull request to the merge queue Nov 5, 2023

linter

1a29f3a

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 5, 2023

igiloh-pinecone and others added 6 commits November 5, 2023 15:33

Merge branch 'dev' into server-docstrings

3582401

linters

457b61b

[context] Simplify ContextContent

50efc68

- Made StuffingContextContent always a List - Slightly improved readability of `StuffingContextBuilder`

[context] StuffingContextContent - Removed special iterator functions

f0b40e9

I changed the tests to use explicit json.loads()

Merge pull request #140 from pinecone-io/cli_server

4e7a2ce

Minor CLI improvements

igiloh-pinecone and others added 4 commits November 6, 2023 13:43

[context] Moved SuffingContextBuilder's data models into the same file

02637da

Makes the code more readable and explicit

[app] /query return type - added ContextResponse model

db9ca51

KISS solution - simply return a different model than the actual internal `Context`

Linter fixes + wrong return type

3c828c4

Merge pull request #144 from pinecone-io/refactor_context_content

2104b65

Context must contain a ContextContent that implements to_text()

igiloh-pinecone enabled auto-merge November 6, 2023 15:02

igiloh-pinecone added this pull request to the merge queue Nov 6, 2023

Merged via the queue into dev with commit 2be988e Nov 6, 2023
10 checks passed

igiloh-pinecone deleted the server-docstrings branch November 6, 2023 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

canopy server documentation #136

canopy server documentation #136

miararoy commented Nov 2, 2023

igiloh-pinecone left a comment

igiloh-pinecone Nov 2, 2023

igiloh-pinecone Nov 2, 2023

nc2112 left a comment

nc2112 Nov 2, 2023

byronnlandry left a comment

byronnlandry Nov 2, 2023

byronnlandry Nov 2, 2023

igiloh-pinecone Nov 5, 2023

igiloh-pinecone left a comment

review-notebook-app bot commented Nov 6, 2023

	description="The document metadata, to learn more about metadata, see https://docs.pinecone.io/docs/manage-data", # noqa: E501
	description="The document metadata. To learn more about metadata, see https://docs.pinecone.io/docs/manage-data", # noqa: E501

	role: Role = Field(description="The role of the messages author.")
	role: Role = Field(description="The role of the message author.")

canopy server documentation #136

canopy server documentation #136

Conversation

miararoy commented Nov 2, 2023

Problem

Solution

Type of Change

Test Plan

igiloh-pinecone left a comment

Choose a reason for hiding this comment

igiloh-pinecone Nov 2, 2023

Choose a reason for hiding this comment

igiloh-pinecone Nov 2, 2023

Choose a reason for hiding this comment

nc2112 left a comment

Choose a reason for hiding this comment

nc2112 Nov 2, 2023

Choose a reason for hiding this comment

byronnlandry left a comment

Choose a reason for hiding this comment

byronnlandry Nov 2, 2023

Choose a reason for hiding this comment

byronnlandry Nov 2, 2023

Choose a reason for hiding this comment

igiloh-pinecone Nov 5, 2023

Choose a reason for hiding this comment

igiloh-pinecone left a comment

Choose a reason for hiding this comment

review-notebook-app bot commented Nov 6, 2023