Skip to content

Conversation

@MengjinYan
Copy link
Contributor

Thank you for contributing to Ray! 🚀
Please review the Ray Contribution Guide before opening a pull request.

⚠️ Remove these instructions before submitting your PR.

💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete.

Description

Briefly describe what this PR accomplishes and why it's needed.

Related issues

Link related issues: "Fixes #1234", "Closes #1234", or "Related to #1234".

Additional information

Optional: Add implementation details, API changes, usage examples, screenshots, etc.

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a long sleep and a || true to a CI test step, likely for debugging purposes as indicated by the '[Do not merge]' in the title. While useful for investigation, these changes would be critical if merged. The || true would mask test failures, and the sleep command would block the CI agent for over 27 hours. My review highlights the need to revert these changes before any potential merge.

Comment on lines +162 to +163
--test-env=USERPROFILE || true
- sleep 100000
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

These changes appear to be for debugging a CI failure, as suggested by the pull request title. However, they introduce critical issues if merged:

  1. Masked Test Failures: The || true on line 162 will cause the bazel test command to always exit with a success code, effectively hiding any real test failures. The soft_fail: true setting is the correct way to handle non-blocking failures for this step.
  2. Blocked CI Agent: The sleep 100000 on line 163 will cause the CI agent to hang for over 27 hours, preventing other jobs from running.

While these are valid debugging techniques, please ensure they are removed before this branch is merged.

        --test-env=USERPROFILE

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>
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.

Ray fails to serialize self-reference objects

1 participant