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

Serve resources from API routes ending in / #926

Merged
merged 4 commits into from
Aug 4, 2023

Conversation

beggers
Copy link
Contributor

@beggers beggers commented Aug 3, 2023

Description of changes

#895

Serve registered API routes regardless of whether they end in "/".

In the linked bug @HammadB and I discussed looping over all the routes at the end of __init__, but the typechecker complains: the only thing we know about self._app.routes is that they're instances of BaseRoute, which may not have the fields we need. Rather than do type coercion I subclassed fastapi.APIRouter.

The logic to set kwargs["include_in_schema"] is maybe too clever, should we just do an if-else instead?

Test plan

0 ~ beggers % curl http://localhost:8000/api/v1/collections/ -v
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /api/v1/collections/ HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/8.1.2
> Accept: */*
> 
< HTTP/1.1 200 OK
< date: Thu, 03 Aug 2023 19:14:01 GMT
< server: uvicorn
< content-length: 2
< content-type: application/json
< 
* Connection #0 to host localhost left intact
[]%                                                                                   
0 ~ beggers % curl http://localhost:8000/api/v1/collections -v 
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /api/v1/collections HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/8.1.2
> Accept: */*
> 
< HTTP/1.1 200 OK
< date: Thu, 03 Aug 2023 19:14:02 GMT
< server: uvicorn
< content-length: 2
< content-type: application/json
< 
* Connection #0 to host localhost left intact
[]%

Also, pytest locally and CI. I'd like to confirm this doesn't break JS client generation and doc generation -- how can I do that?

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

No documentation changes needed for this.

@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readbility, Modularity, Intuitiveness)

@HammadB
Copy link
Collaborator

HammadB commented Aug 3, 2023

Thanks!

Docs generation readme is here - https://github.com/chroma-core/docs#generating-python-docs (and right below for js)
Client generation readme is here - https://github.com/chroma-core/chroma/blob/main/clients/js/DEVELOP.md

I'd expect there to be no diff in either when we rerun them, thanks for checking

@@ -69,6 +69,29 @@ def _uuid(uuid_str: str) -> UUID:
raise InvalidUUIDError(f"Could not parse {uuid_str} as a UUID")


class APIRouter(fastapi.APIRouter):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit - let's not stomp the fastapi name for the class

"include_in_schema" in kwargs and not kwargs["include_in_schema"]
)

kwargs["include_in_schema"] = not exclude_from_schema and not path.endswith("/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

extreme nit - duplicated logic (feel free to ignore this, just calling it out)

Copy link
Collaborator

@HammadB HammadB left a comment

Choose a reason for hiding this comment

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

Approved pending nits, the testing we discussed and CI! Thank you!

@beggers
Copy link
Contributor Author

beggers commented Aug 4, 2023

  • Python doc generation seems broken; fix python docs docs#105 should fix it? When I run yarn gen-python it does make some changes before it breaks, but those changes all appear to be in Jeff's PR.
  • Regenerating JS docs picks up some formatting strays, but nothing related to this change.
  • No diff in the JS client when I run yarn build (the docs say yarn dev but that isn't in package.json).

I'm fairly confident I checked all of those correctly but not 100%. So if this causes trouble down the line feel free to revert and @ me.

Also addressed your other comments :^)

@jeffchuber
Copy link
Contributor

@beggers yep that PR cleans it up :)

@HammadB
Copy link
Collaborator

HammadB commented Aug 4, 2023

@beggers looks great! I'll merge after CI passes shortly.

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.

3 participants