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

Fix R CRAN failures. #7404

Merged
merged 4 commits into from Nov 16, 2021
Merged

Fix R CRAN failures. #7404

merged 4 commits into from Nov 16, 2021

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Nov 8, 2021

Related: #7260 (comment)

  • Remove hist builder dtor.
    I did not reproduce the warning using clang-14, might be missing some flags in my config or it's just a bug in clang-12 used by CRAN test. Anyway, this is a cleanup that removes unnecessary code.
  • Initialize some variables.

@trivialfis trivialfis mentioned this pull request Nov 8, 2021
8 tasks
@trivialfis trivialfis requested a review from hcho3 November 8, 2021 13:55
@hetong007
Copy link
Member

Thanks. May I suggest to hold this PR until we see all the checks for 1.5.0.1 on this page: https://cran.r-project.org/web/checks/check_results_xgboost.html. It might take a few days, and we have until Nov 22nd to patch.

@hetong007
Copy link
Member

hetong007 commented Nov 9, 2021

There are additional issues with valgrind: https://www.stats.ox.ac.uk/pub/bdr/memtests/valgrind/xgboost/00check.log:

  • An R test failure: Failure (test_helpers.R:231:11): xgb-attribute numeric precision
  • Some leaks in the end of the check log.

I didn't remember CRAN has complained about valgrind issues (let's see if they do this time), but if it is fixable on our side maybe we can also try?

@trivialfis
Copy link
Member Author

trivialfis commented Nov 9, 2021

An R test failure: Failure (test_helpers.R:231:11): xgb-attribute numeric precision

  • That's just testing the as.numeric in R. XGBoost is not involved. Will relax the test. A longer-term solution might be using JSON instead.

I did not reproduce the error using valgrind. The patch simply adds tolerance.

at 0x1E1906EF: xgboost::data::SimpleDMatrix::GetGradientIndex(xgboost::BatchParam const&) (packages/tests-vg/xgboost/src/amalgamation/../src/data/simple_dmatrix.cc:94)

  • Probably the gpu_id, which is not used in that context. Will fix.

0x1E21680E: xgboost::DMatrix* xgboost::DMatrix::Createxgboost::data::DenseAdapter(xgboost::data::DenseAdapter*, float, int, std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&) (packages/tests-vg/xgboost/src/amalgamation/../src/data/data.cc:828)

Need to find out what's not initialized.

==2937178== by 0x1E148116: XGDMatrixCreateFromCSC_R (packages/tests-vg/xgboost/src/xgboost_R.cc:137)

Leak in libgomp? Not sure if there's anything we can do.

XGDMatrixSetInfo_R (packages/tests-vg/xgboost/src/xgboost_R.cc:203)

std::vector should manage the memory as RAII. Not sure if there's anything we can do.

@trivialfis trivialfis changed the title Remove hist builder dtor. Fix R CRAN failures. Nov 9, 2021
@trivialfis
Copy link
Member Author

trivialfis commented Nov 9, 2021

Ran valgrind with Rscript testthat.R. No error or warning from valgrind.

valgrind: 3.15.0
Rscript: 3.6.3
gcc: 9.3.0

@trivialfis
Copy link
Member Author

@hcho3 Do you think we can free some budget for running valgrind with gtest on CI once we remove CUDA10?

@hcho3
Copy link
Collaborator

hcho3 commented Nov 9, 2021

@trivialfis We can just use GitHub Actions for this?

@trivialfis
Copy link
Member Author

But then we will skip all GPU code path.

@hcho3
Copy link
Collaborator

hcho3 commented Nov 9, 2021

@trivialfis I don't think CRAN tests are run for any GPU code path

@trivialfis
Copy link
Member Author

running valgrind with gtest

I want to run valgrind with c++ tests. But there are some warnings whenever GPU code is executed, so we might just limit it to CPU only for now:

==304308== Warning: noted but unhandled ioctl 0x19 with no size/direction hints.
==304308==    This could cause spurious value errors to appear.
==304308==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==304308== Warning: set address range perms: large range [0x10006000000, 0x10106000000) (noaccess)
==304308== Warning: noted but unhandled ioctl 0x49 with no size/direction hints.
==304308==    This could cause spurious value errors to appear.
==304308==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==304308== Warning: noted but unhandled ioctl 0x21 with no size/direction hints.
==304308==    This could cause spurious value errors to appear.
==304308==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==304308== Warning: noted but unhandled ioctl 0x1b with no size/direction hints.
==304308==    This could cause spurious value errors to appear.
==304308==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
==304308== Warning: noted but unhandled ioctl 0x44 with no size/direction hints.
==304308==    This could cause spurious value errors to appear.
==304308==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.

@trivialfis
Copy link
Member Author

I think this PR should contain all the fixes in XGBoost.

@trivialfis
Copy link
Member Author

@hetong007 I looked into the 2 memory leak reports, first one is from libgomp, the second one is from the std::vector implementation in GCC. I don't know how to fix them and so far haven't been able to reproduce them using my own setup. Any chance we can continue the submission?

@hetong007
Copy link
Member

How about this: let's wait for all results from different platforms in the check page, and fix whatever we can, then submit a patch (1.5.0.2). After this patch, let's see how CRAN may complaint further.

@trivialfis
Copy link
Member Author

Got it. Thanks for guiding the release.

@trivialfis trivialfis added this to 1.5.1 In progress in 2.0 Roadmap Nov 11, 2021
@hetong007
Copy link
Member

I see most CRAN results are available with no new issue. I'm merging this PR.

@hetong007 hetong007 merged commit b0015fd into dmlc:master Nov 16, 2021
2.0 Roadmap automation moved this from 1.5.1 In Progress to 1.6 Done Nov 16, 2021
@trivialfis trivialfis deleted the hist-builder-dtor branch November 16, 2021 03:07
@trivialfis
Copy link
Member Author

Thanks, I will backport it and bump the version

trivialfis added a commit to trivialfis/xgboost that referenced this pull request Nov 16, 2021
* Remove hist builder dtor.

* Initialize values.

* Tolerance.

* Remove the use of nthread in col maker.
@trivialfis trivialfis moved this from 1.6 Done to 1.5.1 Done in 2.0 Roadmap Nov 16, 2021
trivialfis added a commit that referenced this pull request Nov 17, 2021
* Remove hist builder dtor.

* Initialize values.

* Tolerance.

* Remove the use of nthread in col maker.
@hetong007
Copy link
Member

Seems CRAN checks are all finished. Please instruct when to submit (before 22nd).

@trivialfis
Copy link
Member Author

I will run through all the tests again today

trivialfis added a commit to trivialfis/xgboost that referenced this pull request Nov 19, 2021
* Remove hist builder dtor.

* Initialize values.

* Tolerance.

* Remove the use of nthread in col maker.
trivialfis added a commit that referenced this pull request Nov 19, 2021
* Remove hist builder dtor.

* Initialize values.

* Tolerance.

* Remove the use of nthread in col maker.
@trivialfis
Copy link
Member Author

@hetong007 Let me know if there's anything else I can do to help.

@hetong007
Copy link
Member

It is undergoing checks by the CRAN team. Will update here.

@hetong007
Copy link
Member

hetong007 commented Nov 22, 2021

On CRAN now. From the status page I see the row with r-devel-linux-x86_64-fedora-clang has its previous WARN fixed. Appreciated!

@trivialfis
Copy link
Member Author

Excellent!

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

3 participants