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

add ChunkKV #51

Merged
merged 5 commits into from
Mar 5, 2025
Merged

add ChunkKV #51

merged 5 commits into from
Mar 5, 2025

Conversation

Dominic789654
Copy link
Contributor

PR Description

Add ChunkKV paper implementation (https://arxiv.org/abs/2502.00299)

Benchmark Results

Results on Ruler (4096) dataset under different compression ratios (0.1, 0.25, 0.5):

Task 0.1 0.25 0.5
cwe 99.66 99.48 97.14
fwe 94.47 94.33 93.53
niah_multikey_1 99.0 95.4 82.4
niah_multikey_2 86.8 65.6 41.8
niah_multikey_3 86.8 60.4 27.4
niah_multiquery 98.85 93.55 76.85
niah_multivalue 98.15 91.05 77.6
niah_single_1 100.0 100.0 100.0
niah_single_2 99.2 97.6 90.0
niah_single_3 89.6 71.8 51.8
qa_1 87.8 86.0 82.6
qa_2 61.0 60.4 76.85
vt 99.88 99.88 77.6
Average 92.40 85.81 75.04

New press checklist

  • I added mypress_press.py in the presses directory
  • I added MyPress in __init__.py
  • I updated the README.md with a 1 liner about my new press in the Available presses section
  • I added my press in the default_presses list in tests/default_presses.py

@maxjeblick maxjeblick self-requested a review February 20, 2025 10:27
Copy link
Collaborator

@maxjeblick maxjeblick left a comment

Choose a reason for hiding this comment

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

Thanks a lot for opening this PR!
The overall code looks good, thanks for adding it. There are several points that I'd ask to address before merging:

  • We prefer to have a new press class (ChunkKVPress) associated with your paper, as your method (selecting whole chunks) differs from ChunkPress's idea (compress each chunk independently). Both methods also have different default parameters (e.g. chunk_length).
  • After introducing a new class, you can add a new test (similar to test_chunk_press). Please do not modify the existing test that uses ExpectedAttentionPress.
  • Please add your class in the corresponding README section (available presses).
  • Most style errors can be fixed my running make format. You can test style locally by make style. Feel free to add # noqa: flags if necessary; e.g. sometimes mypy has issues to resolve tensor attributes correctly.
  • Please don't forget to sign off your commit. See https://github.com/NVIDIA/kvpress/blob/main/CONTRIBUTING.md for additional information.

Feel free to reach out with any questions or comments!

@Dominic789654
Copy link
Contributor Author

Hi!

Thanks for the review and suggestions! I've made the following changes as requested:

  1. Created a separate ChunkKVPress class instead of modifying ChunkPress with the global_scoring parameter
  2. Added appropriate documentation in README.md to distinguish between the two approaches
  3. Added unit tests for the new ChunkKVPress class
  4. Style check and commit with -s

Copy link
Collaborator

@maxjeblick maxjeblick left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the updates, code looks good!
Could you shorten your description of ChunkKVPress in the README (see also other descriptions)?
Also, there's a minor issue with style checks, it should be easy to fix.

Copy link
Collaborator

@maxjeblick maxjeblick left a comment

Choose a reason for hiding this comment

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

Thanks a lot for integrating ChunkKVPress, LGTM!

@maxjeblick
Copy link
Collaborator

@Dominic789654
To sign off all your commits, you can e.g.

In your local branch, run: git rebase HEAD~1 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin feature/plugin-support

see DS4SD/docling#772 (comment)

@Dominic789654 Dominic789654 force-pushed the ChunkKV branch 2 times, most recently from 3f4332f to 4fb6d1b Compare March 5, 2025 15:29
Signed-off-by: Dominic789654 <xliu29@gmu.edu>
Signed-off-by: Dominic789654 <xliu29@gmu.edu>
Signed-off-by: Dominic789654 <xliu29@gmu.edu>
Signed-off-by: Dominic789654 <xliu29@gmu.edu>
Signed-off-by: Dominic789654 <xliu29@gmu.edu>
@maxjeblick maxjeblick merged commit fd16d8b into NVIDIA:main Mar 5, 2025
2 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