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.

Sign up for GitHub

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

Jump to bottom

[Data] fix RandomAccessDataset.multiget returning unexpected values for missing keys #44769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Data] fix RandomAccessDataset.multiget returning unexpected values for missing keys #44769

wants to merge 1 commit into from
+14 −10

Conversation

tespent
Copy link
Contributor

@tespent tespent commented Apr 16, 2024 •
edited
Loading

As stated in #44768, the current implementation of multiget based on np.searchsorted does not check for missing keys. I added the required checks and updated unit test for this case.

Why are these changes needed?

Related issue number

Closes #44768

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@tespent tespent mentioned this pull request Apr 16, 2024
Open
@tespent tespent force-pushed the fix/random-access-multiget branch from 42508f7 to f6cdaba Compare April 16, 2024 14:01
@tespent tespent changed the title [Data] fix RandomAccessDataset.multiget for missing keys [Data] fix RandomAccessDataset.multiget returning unexpected values for missing keys Apr 16, 2024
@tespent tespent force-pushed the fix/random-access-multiget branch from f6cdaba to 71f01f0 Compare April 17, 2024 03:03
@tespent tespent force-pushed the fix/random-access-multiget branch from 71f01f0 to ea62f91 Compare April 17, 2024 03:25
@anyscalesam anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) data Ray Data-related issues labels Apr 29, 2024
@anyscalesam anyscalesam added P2 Important issue, but not time-critical and removed triage Needs triage (eg: priority, bug/not-bug, and owning component) labels May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericl ericl Awaiting requested review from ericl

@scv119 scv119 Awaiting requested review from scv119

@c21 c21 Awaiting requested review from c21

@amogkam amogkam Awaiting requested review from amogkam

@scottjlee scottjlee Awaiting requested review from scottjlee

@bveeramani bveeramani Awaiting requested review from bveeramani

@raulchen raulchen Awaiting requested review from raulchen

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang

@omatthew98 omatthew98 Awaiting requested review from omatthew98

At least 1 approving review is required to merge this pull request.

Assignees

@c21 c21

Labels
data Ray Data-related issues P2 Important issue, but not time-critical
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Data] RandomAccessDataset.multiget return unexpected values for missing keys.
3 participants
@tespent @c21 @anyscalesam