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

[release] update if xgboost test suite requires result or not. #32340

Merged
merged 6 commits into from
Feb 9, 2023

Conversation

xwjiang2010
Copy link
Contributor

@xwjiang2010 xwjiang2010 commented Feb 8, 2023

Some xgboost tests don't generate result, and we should not fail those tests. This is to fix a regression introduced by https://github.com/ray-project/ray/pull/32055/files

See failing case: https://buildkite.com/ray-project/release-tests-branch/builds/1381#0186332c-8e3c-4199-9315-359a6e1fecf9

Signed-off-by: xwjiang2010 xwjiang2010@gmail.com

Why are these changes needed?

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

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>
Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>
@xwjiang2010
Copy link
Contributor Author

Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

It looks like we just need to change alert to default in the release_tests.yaml for those tests? Not sure if we need any python code changes. Would be great if we could avoid adding special cases there.

@xwjiang2010
Copy link
Contributor Author

xwjiang2010 commented Feb 9, 2023

It looks like we just need to change alert to default in the release_tests.yaml for those tests? Not sure if we need any python code changes. Would be great if we could avoid adding special cases there.

some of the xgboost tests do require result, which is captured in the require_result() function logic. For those tests, if result is fetched wrong, we would want to surface infra error instead of application error.

@Yard1
Copy link
Member

Yard1 commented Feb 9, 2023

Ok but we can just change it to alert: default for the tests that do not require it (xgboost_distributed_api_test, xgboost_ft_small_elastic, xgboost_ft_small_non_elastic), right? alert is set on the test level, after all.

This reverts commit 3140401.
Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>
@xwjiang2010
Copy link
Contributor Author

Ok but we can just change it to alert: default for the tests that do not require it (xgboost_distributed_api_test, xgboost_ft_small_elastic, xgboost_ft_small_non_elastic), right? alert is set on the test level, after all.

ah updated.

Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

Thanks!

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>
@xwjiang2010 xwjiang2010 merged commit 5c1c888 into ray-project:master Feb 9, 2023
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
…roject#32340)

* [release] update if xgboost test suite require result or not.

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>

* format

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>

* Revert "format"

This reverts commit 3140401.

* Revert "[release] update if xgboost test suite require result or not."

This reverts commit 03ca1c0.

* change to default alert.

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>

* remove tests from xgboost_tests alerts.

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>

---------

Signed-off-by: xwjiang2010 <xwjiang2010@gmail.com>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
@xwjiang2010 xwjiang2010 deleted the test branch July 26, 2023 19:54
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.

3 participants