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.

Sign up for GitHub

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

Jump to bottom

[ENH] Add rendezvous hashing to go and python. Add Assignment policy to go so it can be used in future commits #1360

Merged
merged 5 commits into from
Nov 17, 2023
Merged

[ENH] Add rendezvous hashing to go and python. Add Assignment policy to go so it can be used in future commits #1360

merged 5 commits into from
Nov 17, 2023

Conversation

HammadB
Copy link
Collaborator

@HammadB HammadB commented Nov 8, 2023 •
edited

Description of changes

Summarize the changes made by this PR.
Adds basic rendezvous hashing to the python and go codebases so it can be used

Test plan

How are these changes tested?
Basic unit tests. Manually tested against k8s.

Documentation Changes

None required

This was referenced Nov 8, 2023
Merged
Merged
@HammadB Graphite App
Copy link
Collaborator Author

HammadB commented Nov 8, 2023 •
edited

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@HammadB HammadB changed the title Add rendezvous hashing to go and python. Add Assignment policy to go so it can be used in future commits [ENH] Add rendezvous hashing to go and python. Add Assignment policy to go so it can be used in future commits Nov 8, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 8, 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
HammadB commented Nov 8, 2023
View reviewed changes
chromadb/utils/rendezvous_hash.py Outdated
@@ -0,0 +1,29 @@
# An implementation of https://en.wikipedia.org/wiki/Rendezvous_hashing
from typing import Callable, cast
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that the go code and the python code never have to agree on a rendezvous hash so its ok that we don't cross-test for now.

Ishiihara
Ishiihara reviewed Nov 8, 2023
View reviewed changes
go/coordinator/internal/utils/rendezvous_hash.go Show resolved Hide resolved
go/coordinator/internal/utils/rendezvous_hash.go Show resolved Hide resolved
go/coordinator/internal/coordinator/assignment_policy.go
"chroma_log_15",
}

type RendezvousAssignmentPolicy struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rendezvous hashing is a good starting point. We need to minimize topic assignment change for the topic assignment as one of the design goal for topic assignment reconfiguration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep!

This was referenced Nov 10, 2023
Merged
Merged
@HammadB HammadB marked this pull request as ready for review November 16, 2023 00:20
Ishiihara
Ishiihara reviewed Nov 16, 2023
View reviewed changes
Copy link
Contributor

@Ishiihara Ishiihara left a comment

Choose a reason for hiding this comment

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

LGTM. I suggest at least adding basic unit tests.

chromadb/utils/rendezvous_hash.py Show resolved Hide resolved
go/coordinator/internal/utils/rendezvous_hash.go Show resolved Hide resolved
@HammadB HammadB force-pushed the hammad/submit_embedding_record branch 2 times, most recently from a494cf4 to 78abb33 Compare November 16, 2023 22:05
@HammadB HammadB force-pushed the hammad/rendezvous_hashing branch 2 times, most recently from a579403 to 4c660e2 Compare November 16, 2023 22:53
@HammadB HammadB force-pushed the hammad/submit_embedding_record branch from 8886326 to 72a39b5 Compare November 16, 2023 22:58
@HammadB HammadB changed the base branch from hammad/submit_embedding_record to main November 16, 2023 23:41
@HammadB HammadB enabled auto-merge (squash) November 16, 2023 23:42
@HammadB HammadB enabled auto-merge (squash) November 16, 2023 23:43
@HammadB HammadB enabled auto-merge (squash) November 17, 2023 00:03
@HammadB HammadB merged commit 9191799 into main Nov 17, 2023
189 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ishiihara Ishiihara Ishiihara approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@HammadB @Ishiihara