-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[Data][doc] Update on ray.data.Dataset.map() type hints #52455
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
Conversation
Signed-off-by: chuang0221 <chuangellow@gmail.com>
|
During investigation, I found that # In block.py
class _CallableClassProtocol(Protocol[T, U]):
def __call__(self, __arg: T) -> Union[U, Iterator[U]] # Iterator not actually supported for map()Since this is an internal implementation detail not exposed in the public API, I'm keeping this PR focused on fixing the user-facing documentation. The internal type hint inconsistency could be addressed separately if needed. |
python/ray/data/dataset.py
Outdated
| fn: UserDefinedFunction[Dict[str, Any], Dict[str, Any]], | ||
| fn: Union[ | ||
| Callable[[Dict[str, Any]], Dict[str, Any]], | ||
| "_CallableClassProtocol", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I don't think we should expose _CallableClassProtocol as a public typehint since this is an internal class.
Can we use instead just the Callable[[Dict[str, Any]], Dict[str, Any]]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the original design here - since UserDefinedFunction is used in other type hints, was there a specific reason for including the internal class _CallableClassProtocol rather than just using Callable in the UserDefinedFunction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, i don't have the context there unfortunately.
Signed-off-by: chuang0221 <chuangellow@gmail.com>
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically closed because there has been no more activity in the 14 days Please feel free to reopen or open a new pull request if you'd still like this to be addressed. Again, you can always ask for help on our discussion forum or Ray's public slack channel. Thanks again for your contribution! |
…52455) ## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes ray-project#52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Zhiqiang Ma <zhiqiang.ma@intel.com>
…52455) ## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes ray-project#52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: zac <zac@anyscale.com>
## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes #52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
…52455) ## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes ray-project#52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Marco Stephan <marco@magic.dev>
## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes #52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes #52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
…52455) ## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes ray-project#52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
…52455) ## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes ray-project#52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
…52455) ## Why are these changes needed? Currently, the type hints for `Dataset.map()` suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users. This PR updates the type hints to accurately reflect supported types. ## Related issue number Closes ray-project#52279 ## 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 :( --------- Signed-off-by: chuang0221 <chuangellow@gmail.com> Co-authored-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Future-Outlier <eric901201@gmail.com>
Why are these changes needed?
Currently, the type hints for
Dataset.map()suggest it supports both direct returns and generators, but in practice generators are not supported and raise errors. This is misleading for users.This PR updates the type hints to accurately reflect supported types.
Related issue number
Closes #52279
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.