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

Revert "Revert "[serialization] Enable debugging into pickle backend (#23854)"(#23877)" #23878

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Revert "Revert "[serialization] Enable debugging into pickle backend (#23854)"(#23877)" #23878

merged 2 commits into from
Apr 14, 2022

Conversation

suquark
Copy link
Member

@suquark suquark commented Apr 13, 2022 •
edited
Loading

Why are these changes needed?

This enables debugging into pickle backend when using Ray (with the usage in the doc). This is very helpful when facing complicated serialization errors.

We removed the last line of code that was added to the original PR (#23854) accidentally. That line of code causes CI failures in windows and macos.

Related issue number

Checks

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

@suquark suquark changed the title Revert "Revert "[serialization] Enable debugging into pickle backend (#23854)" (#23877)" Revert "Revert "[serialization] Enable debugging into pickle backend (#23854)"" Apr 13, 2022
@suquark suquark changed the title Revert "Revert "[serialization] Enable debugging into pickle backend (#23854)"" Revert "Revert "[serialization] Enable debugging into pickle backend (#23854)"(#23877)" Apr 13, 2022
clarkzinzow
clarkzinzow approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

LGTM now that that extra line is no longer there! 😁

We should double-check the CI results to see if there are any serialization failures, and if it looks good, then this looks good to merge.

@jjyao
Copy link
Collaborator

jjyao commented Apr 13, 2022

Could you add PR description about what's changed from the original PR?

@jjyao jjyao added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Apr 13, 2022
@suquark suquark removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Apr 13, 2022
@suquark
Copy link
Member Author

suquark commented Apr 13, 2022

I added the PR description and changes. All tests pass.

@suquark suquark requested a review from jjyao April 13, 2022 20:12
@suquark
Copy link
Member Author

suquark commented Apr 14, 2022

All CI tests pass

@suquark suquark merged commit 85542c9 into ray-project:master Apr 14, 2022
@suquark suquark deleted the fix_debug branch April 14, 2022 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@jjyao jjyao Awaiting requested review from jjyao

Assignees

@jjyao jjyao

@clarkzinzow clarkzinzow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@suquark @jjyao @clarkzinzow