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

[CLN] ServerAPI to use Collection model #2300

Merged
merged 14 commits into from
Jul 9, 2024
Merged

Conversation

atroyn
Copy link
Contributor

@atroyn atroyn commented Jun 6, 2024

Description of changes

This PR refactors the ServerAPI classes so that their methods only return the Collection model, not the Collection object itself, i.e. the view on the data.

This is a much cleaner separation of concerns than we had before, and should enable faster iteration in parts. For example, the ServerAPI no longer needs to know about EmbeddingFunction or DataLoader etc - these only exist for Clients.

We will probably re-add them when we start moving some of that functionality server-side for cloud, but for now this is a fine solution.

Most the LoC in this PR are changing tests to use the ClientAPI and fixture instead of the ServerAPI and fixture. This does put the client in the line, but I think this is fine.

Test plan

Tests pass in CI.

Documentation Changes

N/A

Copy link

vercel bot commented Jun 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
chroma ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2024 8:19pm

Copy link

github-actions bot commented Jun 6, 2024

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 (Readability, Modularity, Intuitiveness)

Copy link
Contributor Author

atroyn commented Jun 6, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @atroyn and the rest of your teammates on Graphite Graphite

chromadb/api/__init__.py Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jun 6, 2024

Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas

chromadb/api/__init__.py Outdated Show resolved Hide resolved
chromadb/api/fastapi.py Outdated Show resolved Hide resolved
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.

I think the change in test_persist may be an accident? Otherwise just a clean up nit and looks great

@atroyn atroyn force-pushed the anton/propagate-collecton-model branch 4 times, most recently from b4a93c0 to 8a6cde4 Compare July 2, 2024 00:13
@atroyn atroyn mentioned this pull request Jul 2, 2024
@atroyn atroyn force-pushed the anton/propagate-collecton-model branch 2 times, most recently from 3b5338e to b6d0b05 Compare July 2, 2024 18:18
@atroyn atroyn force-pushed the anton/propagate-collecton-model branch from b6d0b05 to b1895df Compare July 9, 2024 01:40
@atroyn atroyn force-pushed the anton/propagate-collecton-model branch from b1895df to 62d584f Compare July 9, 2024 02:02
Copy link
Contributor Author

atroyn commented Jul 9, 2024

Merge activity

  • Jul 8, 11:00 PM EDT: @atroyn started a stack merge that includes this pull request via Graphite.
  • Jul 8, 11:01 PM EDT: @atroyn merged this pull request with Graphite.

@atroyn atroyn merged commit f636e71 into main Jul 9, 2024
66 checks passed
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