Raise instead of hanging on a probability-0 source without replacement - #8708
Open
Arthur031221 wants to merge 1 commit into
Open
Arthur031221 wants to merge 1 commit into
Arthur031221 wants to merge 1 commit into
Conversation
interleave_datasets with probabilities and stopping_strategy="all_exhausted_without_replacement" stops only once every source is exhausted. A source with probability 0 is never drawn, so the sampling loop never ended. Raise the same ValueError that all_exhausted already raises for this case.
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
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.
Anyone who passes
probabilitieswith a 0 weight together withstopping_strategy="all_exhausted_without_replacement"tointerleave_datasetson map-style datasets gets a call that never returns: the source with probability 0 is never drawn, so it is never marked exhausted and the sampling loop keeps drawing forever.On main (fa995bd) and on 5.0.1 this does not return (killed by
timeout 60). #8318 added a clearValueErrorfor the same case underall_exhausted, but that check sits in the branch that handles onlyfirst_exhaustedandall_exhausted, so the without replacement loop still spins.This adds the same check at the top of the without replacement branch. The call above now raises:
The
first_exhaustedandall_exhaustedcode paths are not touched.I went with raising rather than treating probability-0 sources as already exhausted, to match what #8318 chose for
all_exhausted(its test expects aValueErroreven when the probability-0 source is empty). If you would rather skip such sources, initialisingis_exhaustedfromprobabilities == 0is a one-line alternative (the call above then returns[0, 1, 2]) and I can switch to it. #8627 fixes empty sources in this same branch; that is a separate condition from this one.Test:
test_interleave_datasets_probabilities_zero_probability_all_exhausted_raisesis now parametrized over both strategies and matches the message. With the source change reverted, the two newall_exhausted_without_replacementcases time out (pytest-timeout at 60s) and the twoall_exhaustedcases pass; with the change all four pass.The streaming path (
IterableDataset) has the same stall underall_exhausted_without_replacement: it yields the three rows of the first source and then blocks. I kept this PR to the map-style path.