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

[Data] Throw exception for non-streaming HF datasets with "override_num_blocks" argument #47559

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

xingyu-long
Copy link
Contributor

Why are these changes needed?

As in the issue #47507, from_huggingface() does not support override_num_blocks for non-streaming HF Datasets, so we should throw exception, also we need to pass other arguments for from_huggingface() if they are using streaming dataset

Related issue number

Close #47507

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 :(

I did manual test on exception part. Let me know if I need to do more tests.

@xingyu-long xingyu-long changed the title Throw exception for non-streaming HF datasets with "override_num_blocks" argument [Data] Throw exception for non-streaming HF datasets with "override_num_blocks" argument Sep 7, 2024
@scottjlee scottjlee self-assigned this Sep 9, 2024
…ks" argument

Signed-off-by: Xingyu Long <xingyulong97@gmail.com>
Copy link
Contributor

@scottjlee scottjlee left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

@scottjlee scottjlee added the go add ONLY when ready to merge, run all tests label Sep 11, 2024
@scottjlee scottjlee enabled auto-merge (squash) September 11, 2024 18:53
update the error message

Co-authored-by: Scott Lee <scottjlee@users.noreply.github.com>
Signed-off-by: Xingyu Long <xingyulong97@gmail.com>
auto-merge was automatically disabled September 11, 2024 23:26

Head branch was pushed to by a user without write access

@xingyu-long
Copy link
Contributor Author

Have to trigger another build. I tested it (python/ray/data/tests/test_csv.py::test_csv_read_filter_non_csv_file) locally and passed. @scottjlee could you approve the merge? thanks!

@scottjlee scottjlee enabled auto-merge (squash) September 12, 2024 00:08
@scottjlee scottjlee merged commit 44dd9a7 into ray-project:master Sep 12, 2024
6 checks passed
@xingyu-long xingyu-long deleted the issue_47507 branch September 12, 2024 02:06
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
…um_blocks" argument (ray-project#47559)

## Why are these changes needed?

As in the issue ray-project#47507, from_huggingface() does not support
override_num_blocks for non-streaming HF Datasets, so we should throw
exception, also we need to pass other arguments for from_huggingface()
if they are using streaming dataset

## Related issue number
Close ray-project#47507

## Checks

- [x] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [x] 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 :(

I did manual test on exception part. Let me know if I need to do more
tests.

---------

Signed-off-by: Xingyu Long <xingyulong97@gmail.com>
Co-authored-by: Scott Lee <scottjlee@users.noreply.github.com>
Signed-off-by: ujjawal-khare <ujjawal.khare@dream11.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Data] from_huggingface() does not support override_num_blocks for non-streaming HF Datasets
2 participants