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

build: Remove mmh3 dependency #4896

Merged
merged 14 commits into from
May 17, 2023
Merged

build: Remove mmh3 dependency #4896

merged 14 commits into from
May 17, 2023

Conversation

julian-risch
Copy link
Member

@julian-risch julian-risch commented May 12, 2023

Related Issues

Proposed Changes:

  • Removed mmh3 dependency and added a python implementation of the only function we use from mmh3
  • Hash value remains the same as before with mmh3 (new unit test)

How did you test it?

Notes for the reviewer

Speed is as expected one to two orders of magnitude slower but it's not the bottleneck anyway. Calculating embeddings is much slower. We shouldn't optimize hashing speed here but get rid of the dependency instead.
You can already do the review despite the failing code coverage checks.

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@julian-risch julian-risch requested a review from a team as a code owner May 12, 2023 11:15
@julian-risch julian-risch requested review from ZanSara and removed request for a team May 12, 2023 11:15
@coveralls
Copy link
Collaborator

coveralls commented May 12, 2023

Coverage Status

Coverage: 38.539% (+0.3%) from 38.239% when pulling 9ea7c3c on remove-mmh3 into 9d52998 on main.

Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Great!

@julian-risch julian-risch merged commit 8cfeed0 into main May 17, 2023
74 of 75 checks passed
@julian-risch julian-risch deleted the remove-mmh3 branch May 17, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove mmh3 dependency
3 participants