Merge the 1.1.1 patch release back into main - #437
Open
EmilHvitfeldt wants to merge 15 commits into
Open
Conversation
xgboost 3.4.0 canonicalises booster = "dart" to "gbtree" in the saved JSON while still writing weight_drop. Look for the dropout weights regardless of the serialised booster name, and treat a model as DART whenever non-trivial weights are present. Fixes #238
Backport of the test-only part of 02c87d4: round the snapshotted coefficients and pin the Cox tie-handling method, both of which are sensitive to the glmnet version and platform.
Two problems showed up when running against xgboost 3.4.1.1: rate_drop alone leaves dropout to the RNG, and 3.4 drops no tree at all for the configuration these tests used, so weight_drop came back all ones and the dropout path was never exercised. Force a drop per round with one_drop. The mtcars + 0.1 offset was meant to keep test data clear of split boundaries, but xgboost stores thresholds as float32, and which splits get chosen varies by version. Snap the data to a 1/64 grid instead so every value is float32-exact, which removes the boundary ties on both versions. Also refresh the revdepcheck results.
R CMD check reported "CPU time 5.3 times elapsed time" for the test suite. xgboost, lightgbm and ranger all default to one worker thread per core, and on the tiny toy models used here those threads spend their time idling rather than working, which inflates the CPU-to-elapsed ratio. Cap OpenMP in tests/testthat.R before any modeling package is loaded (libgomp reads OMP_NUM_THREADS at load time, so this has to come first), and additionally pass the thread count explicitly at every fit site since setting the env var is not reliable once libgomp is initialised: * xgboost: nthread = 1 in every xgb.train() params list * lightgbm: num_threads = 1 in every lgb.train() params list * ranger: num.threads = 1 throughout; ranger uses std::thread rather than OpenMP, so it ignores the env var entirely The remaining engines (randomForest, rpart, earth, Cubist, partykit) are single-threaded, and the catboost tests skip on CRAN. One ranger snapshot is re-accepted because the printed call now shows num.threads = 1.
d496471 passed thread counts to xgb.train(), lgb.train() and ranger(), but three other entry points default to one thread per core and were still uncapped: * xgb.DMatrix(nthread = NULL) builds the matrix using every core, so all 24 call sites now pass nthread = 1. * lgb.Dataset() takes its thread count from `params`, which none of the 46 call sites supplied. * predict.ranger(num.threads = NULL) uses every core regardless of what the fit used. ranger uses std::thread, so no OMP_* variable reaches it. lightgbm also calls omp_set_num_threads() itself when predicting, which overrides the environment, so the 30 predict() calls pass num_threads through `params` too.
This is what actually caused the "CPU time 3.1 times elapsed time" NOTE. Debian and Ubuntu default to the pthread build of OpenBLAS, which uses every core and deliberately ignores OMP_NUM_THREADS, so every environment variable set here so far was inert on the machine that reported the NOTE. earth() runs thousands of tiny least-squares fits, the only code in the suite that crosses OpenBLAS's threading threshold. Measured on a 4-core Linux runner with openblas-pthread, where a bare dgemm() reports a CPU-to-elapsed ratio of 3.8: * test-model-earth.R went from 1.91 to 1.00 * the whole suite went from [23s/18s] to [13s/13s] OpenBLAS reads its environment when it is loaded, which happens before tests/testthat.R runs, so this needs the in-process call that RhpcBLASctl provides. data.table latches its thread count at load time for the same reason and runs its own pool inside xgb.model.dt.tree() and lgb.model.dt.tree(), neither of which takes a thread argument. Drops OMP_WAIT_POLICY, GOMP_SPINCOUNT and R_DATATABLE_NUM_THREADS again, which were added on a hunch and measured no effect.
Vignette re-building has the same exposure as the tests: mars.Rmd runs earth() through the same multithreaded OpenBLAS, and the xgboost, lightgbm and ranger vignettes fit with default thread counts. Every vignette that fits a model now sources vignettes/_threads.R from its setup chunk, ahead of the library() calls, since libgomp and OpenBLAS both latch their thread counts when they are loaded. Note that vignettes/.gitignore ignores *.R, so _threads.R needs an explicit exception or the vignettes fail to build from a fresh checkout. This caps thread use during the model fitting, confirmed by blas_get_num_procs() reporting 1 in all 13 vignette processes, but it does not move the reported ratio, which stays at [31s/25s]. Each of the 13 processes pays a constant 0.34s of concurrent CPU at startup, when OpenBLAS spawns its thread pool before any package code runs. Nothing in the package can reclaim that; it would take OPENBLAS_NUM_THREADS in the environment before R starts.
Folds the 1.1.1 patch release back into development. Resolutions: - DESCRIPTION: version 1.1.1.9000; keep both branches' Suggests (dbarts from main, data.table and RhpcBLASctl from the release). - NEWS.md: 1.1.1 section below the development section. - README, glmnet and ranger tests and snapshots: keep main's, which already carry the equivalent changes. - revdep/README.md: keep the 1.1.1 run's results. - R/model-xgboost.R: main's objective extraction plus the release branch's weight_drop dropout detection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Folds the 1.1.1 patch release back into development. 1.1.1 was cut from the
v1.1.0tag and released fromcran-1.1.1so that the xgboost 3.4 dropout fix (#238) could reach CRAN without waiting on the rest of the development version.Merge this with a merge commit, not a squash, so that the
v1.1.1tag stays an ancestor ofmain.Conflict resolutions
DESCRIPTION: version set to1.1.1.9000. Suggests keepsdbartsfrommainand gainsdata.tableandRhpcBLASctlfrom the release branch's thread capping.rpartwas already present, so the duplicate entry the release branch would have added is dropped.NEWS.md: the automatic merge had mangledmain's# tidypredict (development version)heading into a second# tidypredict 1.1.0. Restored, with the1.1.1section placed between the development section and the real1.1.0. The#238bullet appears once.R/model-xgboost.R: auto-merged. Verified thatparse_xgb_json_params()carries bothmain's objective extraction and the release branch'sweight_dropdropout detection.README.Rmd,README.md,tests/testthat/_snaps/model-glmnet.md,tests/testthat/test-model-ranger.R,tests/testthat/_snaps/model-ranger.md: keptmain's versions, which already carry the equivalent changes.mainalready caps ranger threads in 17 places.revdep/README.md: took the 1.1.1 run's results, which are newer thanmain's February run.tests/testthat/catboost_info/from the merge. Those are generated artifacts the release branch had tracked, andmain's.gitignorealready excludes them.Verification
Full suite on the merge result:
FAIL 0 | WARN 6 | SKIP 1 | PASS 2679. The six warnings are pre-existing (quantreg and lm perfect-fit notes, an emptymultinomgroup).air format .produced no changes.Follow-up
mainnow has the proper float32 threshold fix from #273, so the two 1/64-grid workarounds intest-model-xgboost.Rare probably redundant and could revert to plainmtcars. Left alone here to keep the merge to a merge.