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

pass write result to on_write_complete #49091

Closed
wants to merge 3 commits into from

Conversation

chenkovsky
Copy link

@chenkovsky chenkovsky commented Dec 5, 2024

Why are these changes needed?

Currently, Datasink will ignore the result of write. It's hard to implement commit logic in on_write_complete.
This PR refactors plan_write_op and changes typing for write & on_write_complete. data sink can pass result of write to on_write_complete

Related issue number

Closes #48933

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

Signed-off-by: Chongchen Chen <[email protected]>
@chenkovsky chenkovsky marked this pull request as ready for review December 6, 2024 02:28
@chenkovsky chenkovsky requested a review from a team as a code owner December 6, 2024 02:28
Signed-off-by: Chongchen Chen <[email protected]>
@raulchen raulchen self-assigned this Dec 12, 2024
@raulchen
Copy link
Contributor

hi @chenkovsky sorry for the delayed review.
One problem with the PR is that certain optimization rules depend on the write op to have 2 separate MapTransformFns: write and stats.
Also I plan to make some minor changes to the DataSink interface.
I'll submit a new PR to address this issue soon.

@chenkovsky chenkovsky closed this Dec 13, 2024
@raulchen
Copy link
Contributor

@chenkovsky #49251 is the new PR

@chenkovsky
Copy link
Author

chenkovsky commented Dec 13, 2024

@chenkovsky #49251 is the new PR

@raulchen
awesome, your PR has clear generic typing. it looks pretty good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data][DataSink] allow passing write results to on_write_complete
2 participants