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

Fixes for the new bulk execution backend #31884

Merged
merged 24 commits into from
Jan 24, 2023

Conversation

jianoaix
Copy link
Contributor

@jianoaix jianoaix commented Jan 24, 2023

Why are these changes needed?

To enable the new bulk execution backend: #30903

Related issue number

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

# TODO(https://github.com/ray-project/ray/issues/31145): re-enable
# after the segfault bug is fixed.
if DatasetContext.get_current().new_execution_backend:
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to a separate file instead of disabling.

Copy link
Contributor Author

@jianoaix jianoaix Jan 24, 2023

Choose a reason for hiding this comment

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

That doesn't work either for this test, bazel still failed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you move this into a separate file, you can also remove the ray.shutdown() code below. That should avoid restarting Ray entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It failed (bazel test python/ray/data:tests/test_pipeline_incremental_take locally) still, but the CI test (which uses bazel) passed, didn't find out why.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there was another Ray running locally or something.

python/ray/data/tests/test_dataset.py Show resolved Hide resolved
@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 24, 2023
@jianoaix jianoaix added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jan 24, 2023
@jianoaix jianoaix removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 24, 2023
@jianoaix
Copy link
Contributor Author

@ericl tests passing, ready to merge, thanks

@ericl ericl merged commit 5682ef9 into ray-project:master Jan 24, 2023
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.

2 participants