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

[train] Storage: Update XGBoost + LightGBM callback APIs #38558

Merged
merged 13 commits into from
Aug 22, 2023

Conversation

krfricke
Copy link
Contributor

Why are these changes needed?

The APIs are currently still using the deprecated tune.checkpoint_dir context manager and tune.report.

This PR updates XGBoost+LightGBM to use train.report instead. It also consolidates the available callbacks into one.

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

Kai Fricke added 2 commits August 17, 2023 16:13
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Kai Fricke added 2 commits August 17, 2023 19:07
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Kai Fricke added 2 commits August 17, 2023 22:48
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

Thanks! Just some minor comments.

Comment on lines 86 to 87
_checkpoint_callback_cls = None
_report_callbacks_cls = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check if these properties are set for backwards compatibility with xgboost-ray and lightgbm-ray (and also for sequencing the API update).

We could test this also with getattr but it makes the code that invokes them more cluttered.

python/ray/tune/integration/lightgbm.py Outdated Show resolved Hide resolved
Kai Fricke added 6 commits August 21, 2023 16:46
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke merged commit 4241263 into ray-project:master Aug 22, 2023
60 of 65 checks passed
@krfricke krfricke deleted the xgboost-api branch August 22, 2023 12:34
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
…#38558)

The APIs are currently still using the deprecated `tune.checkpoint_dir` context manager and `tune.report`.

This PR updates XGBoost+LightGBM to use `train.report` instead. It also consolidates the available callbacks into one.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
…#38558)

The APIs are currently still using the deprecated `tune.checkpoint_dir` context manager and `tune.report`.

This PR updates XGBoost+LightGBM to use `train.report` instead. It also consolidates the available callbacks into one.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Victor <vctr.y.m@example.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.

None yet

2 participants