fix(buildsilver): ValueError when first_n_pages > max_sample_pages - #466
Open
devteamaegis wants to merge 1 commit into
Open
fix(buildsilver): ValueError when first_n_pages > max_sample_pages#466devteamaegis wants to merge 1 commit into
devteamaegis wants to merge 1 commit into
Conversation
When first_n_pages exceeds max_sample_pages and a PDF has more pages than first_n_pages, the additional-pages count (max_sample_pages - first_n_pages) is negative. random.sample() rejects a negative sample size and raises ValueError, crashing page sampling. Clamp the count to be non-negative. Adds tests covering the negative-count case and normal sampling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's broken
sample_pdf_pages()crashes withValueError: Sample larger than population or is negativewhenfirst_n_pages > max_sample_pagesand the PDF has more pages thanfirst_n_pages. The additional-pages countmax_sample_pages - first_n_pagesgoes negative and is passed torandom.sample(), which rejects negative sizes.--first_n_pagesand--max_sample_pagesare independent CLI flags, so e.g.--first_n_pages 20with the default--max_sample_pages 15makes every multi-page PDF crash inprocess_pdf.Why it happens
min(max_sample_pages - first_n_pages, len(remaining_pages))can be negative; nothing floors it at 0. The code assumesmax_sample_pages >= first_n_pages.Fix
Clamp the sample count with
max(0, ...). The firstfirst_n_pagesare still returned; no extra sampling once the budget is exhausted.Test
Adds
tests/test_buildsilver.pycovering the negative-count case (fails before), plus normal sampling and fewer-pages-than-first_n. Passes after.