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

[R-package] suppress warning about empty 'compute' directory and fix Rtools installation #3277

Merged
merged 17 commits into from Aug 9, 2020

Conversation

jameslamb
Copy link
Collaborator

Today, when building the R package using Rscript build_r.R, you will see the following warning:

* checking for empty or unneeded directories
WARNING: directory ‘lightgbm/src/compute/test’ is empty

That warning can be safely ignored...it doesn't mean that anything is wrong. compute/ is only used when building LightGBM with GPU support, and empty directories in it do not cause any issues.

This PR proposes a way to avoid the warning during non-GPU builds of the R package (the most common installation). It think it's valuable to remove these non-meaningful warnings so users do not waste effort investigating them when they have installation issues.

@jameslamb
Copy link
Collaborator Author

using this PR to try to fix #3280 as well

@jameslamb
Copy link
Collaborator Author

just re-started this build to see if the Rtools issues from yesterday have been resolved (#3280 )

@guolinke
Copy link
Collaborator

guolinke commented Aug 9, 2020

maybe also fix this ?

Installing collected packages: docutils, rstcheck, alabaster, sphinxcontrib-serializinghtml, MarkupSafe, Jinja2, pytz, babel, sphinxcontrib-applehelp, chardet, urllib3, idna, requests, sphinxcontrib-jsmath, pyparsing, six, packaging, Pygments, sphinxcontrib-htmlhelp, snowballstemmer, imagesize, sphinxcontrib-devhelp, sphinxcontrib-qthelp, sphinx, sphinx-rtd-theme, breathe
ERROR: After October 2020 you may experience errors when installing or updating packages. This is because pip will change the way that it resolves dependency conflicts.
We recommend you use --use-feature=2020-resolver to test your packages with the new resolver before it becomes the default.

also @StrikerRUS

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented Aug 9, 2020

This is because pip will change the way that it resolves dependency conflicts.

Oh, really!? Maybe we will be able to drop finally that ugly workaround for inconsistent versions of Breathe and Sphinx (refer to #3015 for the background)! I'll check that.

@StrikerRUS StrikerRUS changed the title [R-package] suppress warning about empty 'compute' directory [R-package] suppress warning about empty 'compute' directory and fix Rtools installation Aug 9, 2020
.ci/test_r_package_windows.ps1 Outdated Show resolved Hide resolved

# Install R
Write-Output "Installing R"
Start-Process -FilePath R-win.exe -NoNewWindow -Wait -ArgumentList "/VERYSILENT /DIR=$env:R_LIB_PATH/R /COMPONENTS=main,x64" ; Check-Output $?
Write-Output "Done installing R"

Write-Output "Installing Rtools"
Start-Process -FilePath Rtools.exe -NoNewWindow -Wait -ArgumentList "/VERYSILENT /DIR=$RTOOLS_INSTALL_PATH" ; Check-Output $?
./Rtools.exe /VERYSILENT /SUPPRESSMSGBOXES /DIR=$RTOOLS_INSTALL_PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's try to use previous scheme with Start-Process and see whether only SUPPRESSMSGBOXES can help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least we should place exit code check back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how is Start-Process better than ./something.exe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doesn't matter, I guess. Switching back to Start-Process and adding in SUPPRESSMSGBOXES, the R 4.0 Windows builds are still running after 30 minutes: https://github.com/microsoft/LightGBM/pull/3277/checks?check_run_id=962759622

I'm going to switch to ./Rtools.exe and try adding the exit code check back

Copy link
Collaborator

Choose a reason for hiding this comment

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

how is Start-Process better than ./something.exe?

Here are some thoughts about it: https://social.technet.microsoft.com/wiki/contents/articles/7703.powershell-running-executables.aspx. TL;DR: it gives you full control.

Comment on lines 14 to +15
runs-on: ${{ matrix.os }}
timeout-minutes: 60
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need it in the master.

Suggested change
runs-on: ${{ matrix.os }}
timeout-minutes: 60
runs-on: ${{ matrix.os }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why not? What is the problem with including it?

The default is 6 hours, which is way longer than these jobs need

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are dozens of config params. Including them all will make our file less readable. I believe we should keep configs as much clean and short as it is possible. Normally, our jobs don't need neither 1 hour nor 6 hours. So I don't see any differences why we should change this param from 360 to 60. Pending status more than 20 mins already indicates that something is wrong with our builds (for the current number of tests).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I respectully disagree but it is not worth holding up this blocking PR. GitHub won't allow me to commit this suggestion in the browser, but I'll commit it in a few hours when I am home.

@StrikerRUS
Copy link
Collaborator

@jameslamb I've made a PR in your repo (jameslamb#34) to fix the current Travis failures caused by new Sphinx release: sphinx-doc/sphinx#8088. Please review.

Temporary constrain Sphinx version
@StrikerRUS StrikerRUS merged commit 7be57d7 into microsoft:master Aug 9, 2020
@StrikerRUS
Copy link
Collaborator

@guolinke

maybe also fix this ?

I'll check that.

I've tested pip installations with --use-feature=2020-resolver options and results are fine. Actually, we don't use pip much preferring conda at our CI. So I believe everything should be good in October for us.

diff --git a/.ci/test.sh b/.ci/test.sh
index 435614bb..5a5afbf3 100755
--- a/.ci/test.sh
+++ b/.ci/test.sh
@@ -16,7 +16,7 @@ cd $BUILD_DIRECTORY
 if [[ $TRAVIS == "true" ]] && [[ $TASK == "check-docs" ]]; then
     cd $BUILD_DIRECTORY/docs
     conda install -q -y -n $CONDA_ENV -c conda-forge doxygen
-    pip install --user -r requirements.txt rstcheck
+    pip install --user -r requirements.txt rstcheck --use-feature=2020-resolver
     # check reStructuredText formatting
     cd $BUILD_DIRECTORY/python-package
     rstcheck --report warning `find . -type f -name "*.rst"` || exit -1
@@ -49,7 +49,7 @@ if [[ $TRAVIS == "true" ]] && [[ $TASK == "lint" ]]; then
         -c conda-forge \
             libxml2 \
             "r-lintr>=2.0"
-    pip install --user cpplint
+    pip install --user cpplint --use-feature=2020-resolver
     echo "Linting Python code"
     pycodestyle --ignore=E501,W503 --exclude=./compute,./.nuget . || exit -1
     pydocstyle --convention=numpy --add-ignore=D105 --match-dir="^(?!^compute|test|example).*" --match="(?!^test_|setup).*\.py" . || exit -1
@@ -83,7 +83,7 @@ fi
 
 if [[ $TASK == "sdist" ]]; then
     cd $BUILD_DIRECTORY/python-package && python setup.py sdist || exit -1
-    pip install --user $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz -v || exit -1
+    pip install --user $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz -v --use-feature=2020-resolver || exit -1
     if [[ $AZURE == "true" ]]; then
         cp $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz $BUILD_ARTIFACTSTAGINGDIRECTORY
         mkdir $BUILD_DIRECTORY/build && cd $BUILD_DIRECTORY/build
@@ -115,7 +115,7 @@ elif [[ $TASK == "bdist" ]]; then
             cp dist/lightgbm-$LGB_VER-py2.py3-none-manylinux1_x86_64.whl $BUILD_ARTIFACTSTAGINGDIRECTORY
         fi
     fi
-    pip install --user $BUILD_DIRECTORY/python-package/dist/*.whl || exit -1
+    pip install --user $BUILD_DIRECTORY/python-package/dist/*.whl --use-feature=2020-resolver || exit -1
     pytest $BUILD_DIRECTORY/tests || exit -1
     exit 0
 fi
@@ -127,7 +127,7 @@ if [[ $TASK == "gpu" ]]; then
     grep -q 'std::string device_type = "gpu"' $BUILD_DIRECTORY/include/LightGBM/config.h || exit -1  # make sure that changes were really done
     if [[ $METHOD == "pip" ]]; then
         cd $BUILD_DIRECTORY/python-package && python setup.py sdist || exit -1
-        pip install --user $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz -v --install-option=--gpu --install-option="--opencl-include-dir=$AMDAPPSDK_PATH/include/" || exit -1
+        pip install --user $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz -v --install-option=--gpu --install-option="--opencl-include-dir=$AMDAPPSDK_PATH/include/" --use-feature=2020-resolver || exit -1
         pytest $BUILD_DIRECTORY/tests/python_package_test || exit -1
         exit 0
     fi
@@ -135,7 +135,7 @@ if [[ $TASK == "gpu" ]]; then
 elif [[ $TASK == "mpi" ]]; then
     if [[ $METHOD == "pip" ]]; then
         cd $BUILD_DIRECTORY/python-package && python setup.py sdist || exit -1
-        pip install --user $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz -v --install-option=--mpi || exit -1
+        pip install --user $BUILD_DIRECTORY/python-package/dist/lightgbm-$LGB_VER.tar.gz -v --install-option=--mpi --use-feature=2020-resolver || exit -1
         pytest $BUILD_DIRECTORY/tests/python_package_test || exit -1
         exit 0
     fi
diff --git a/.ci/test_windows.ps1 b/.ci/test_windows.ps1
index fd0e9f95..b957735c 100644
--- a/.ci/test_windows.ps1
+++ b/.ci/test_windows.ps1
@@ -36,7 +36,7 @@ if ($env:TASK -eq "regular") {
 elseif ($env:TASK -eq "sdist") {
   cd $env:BUILD_SOURCESDIRECTORY/python-package
   python setup.py sdist --formats gztar ; Check-Output $?
-  cd dist; pip install @(Get-ChildItem *.gz) -v ; Check-Output $?
+  cd dist; pip install @(Get-ChildItem *.gz) -v --use-feature=2020-resolver ; Check-Output $?
 
   $env:JAVA_HOME = $env:JAVA_HOME_8_X64  # there is pre-installed Zulu OpenJDK-8 somewhere
   Invoke-WebRequest -Uri "https://sourceforge.net/projects/swig/files/swigwin/swigwin-3.0.12/swigwin-3.0.12.zip/download" -OutFile $env:BUILD_SOURCESDIRECTORY/swig/swigwin.zip -UserAgent "NativeHost"
@@ -50,7 +50,7 @@ elseif ($env:TASK -eq "sdist") {
 elseif ($env:TASK -eq "bdist") {
   cd $env:BUILD_SOURCESDIRECTORY/python-package
   python setup.py bdist_wheel --plat-name=win-amd64 --universal ; Check-Output $?
-  cd dist; pip install @(Get-ChildItem *.whl) ; Check-Output $?
+  cd dist; pip install @(Get-ChildItem *.whl) --use-feature=2020-resolver ; Check-Output $?
   cp @(Get-ChildItem *.whl) $env:BUILD_ARTIFACTSTAGINGDIRECTORY
 } elseif (($env:APPVEYOR -eq "true") -and ($env:TASK -eq "python")) {
   cd $env:BUILD_SOURCESDIRECTORY\python-package

I just noticed one warning but it is unrelated and I'll try to fix it today.

C:\Miniconda37-x64\envs\test-env\lib\site-packages\setuptools\distutils_patch.py:26: UserWarning: Distutils was imported before Setuptools. This usage is discouraged and may exhibit undesirable behaviors or errors. Please use Setuptools' objects directly or at least import Setuptools first.
  "Distutils was imported before Setuptools. This usage is discouraged "

@jameslamb
Copy link
Collaborator Author

@StrikerRUS thanks! I agree, I think we will be ok from a user perspective too. I could see our requirement of scikit-learn!=0.22.0 causing issues with the new resolver, but by October that release of scikit-learn will be 10 months old, so hopefully not too many people will be on it.

@jameslamb jameslamb deleted the fix/build-r-warning branch October 11, 2020 04:37
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants