Skip to content

feat: Make it optional to provide splits for Sampler in LoadRequest#127

Merged
felix0097 merged 11 commits intomainfrom
ff/make-splits-optional
Jan 29, 2026
Merged

feat: Make it optional to provide splits for Sampler in LoadRequest#127
felix0097 merged 11 commits intomainfrom
ff/make-splits-optional

Conversation

@felix0097
Copy link
Copy Markdown
Collaborator

This PR makes it optional to provide splits in the annbatch.types.LoadRequest.

The logic is the following:

  • If splits is provided -> use splits
  • If splits is not provided -> do random batching

@felix0097 felix0097 requested a review from ilan-gold January 28, 2026 15:53
@felix0097 felix0097 self-assigned this Jan 28, 2026
@felix0097 felix0097 added the skip-gpu-ci Whether gpu ci should be skipped label Jan 28, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.06%. Comparing base (8f6c156) to head (d8f862c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/annbatch/samplers/_chunk_sampler.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   93.76%   91.06%   -2.70%     
==========================================
  Files          10       10              
  Lines         770      795      +25     
==========================================
+ Hits          722      724       +2     
- Misses         48       71      +23     
Files with missing lines Coverage Δ
src/annbatch/abc/sampler.py 100.00% <100.00%> (ø)
src/annbatch/types.py 100.00% <100.00%> (ø)
src/annbatch/samplers/_chunk_sampler.py 93.33% <66.66%> (-1.91%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@felix0097 felix0097 mentioned this pull request Jan 28, 2026
Copy link
Copy Markdown
Collaborator

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Big question is how to handle shuffle - user problem or ours?

Comment thread src/annbatch/abc/sampler.py Outdated
Comment thread src/annbatch/abc/sampler.py Outdated
Comment thread src/annbatch/abc/sampler.py Outdated
Comment thread src/annbatch/abc/sampler.py Outdated
felix0097 and others added 4 commits January 28, 2026 17:02
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
@felix0097 felix0097 requested a review from ilan-gold January 28, 2026 16:11
Comment thread src/annbatch/abc/sampler.py Outdated
@property
@abstractmethod
def shuffle(self) -> bool:
"""Whether to shuffle data during sampling.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would mention that we are interpreting this as shuffling in-memory, but it is up to the implementer to shuffle from the on-disk

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also the bit about batch_size

Comment thread tests/test_sampler.py Outdated
Comment thread tests/test_sampler.py Outdated
Comment thread src/annbatch/abc/sampler.py Outdated
Comment thread src/annbatch/abc/sampler.py Outdated
Comment thread tests/test_sampler.py Outdated
Comment thread tests/test_sampler.py Outdated
felix0097 and others added 4 commits January 29, 2026 13:21
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
@felix0097 felix0097 requested a review from ilan-gold January 29, 2026 12:28
@felix0097 felix0097 merged commit 352f4f8 into main Jan 29, 2026
11 checks passed
@felix0097 felix0097 deleted the ff/make-splits-optional branch January 29, 2026 13:09
@ilan-gold
Copy link
Copy Markdown
Collaborator

@felix0097 Can you add a changelog entry?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-gpu-ci Whether gpu ci should be skipped

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants