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

Disable auto-download by default + improve test stability #3076

Merged
merged 53 commits into from Nov 2, 2021

Conversation

zoq
Copy link
Member

@zoq zoq commented Oct 16, 2021

Looks like we never disabled the auto-download by default (#2953).

@shrit
Copy link
Member

shrit commented Oct 16, 2021

I am not sure, I think DOWNLOAD_DEPENDENCIES is OFF by default.

@zoq
Copy link
Member Author

zoq commented Oct 16, 2021

It is and we download dependencies if DOWNLOAD_DEPENDENCIES is OFF, which is not correct.

@zoq
Copy link
Member Author

zoq commented Oct 16, 2021

Looks like there is something wrong with the armadillo package detection.

@zoq
Copy link
Member Author

zoq commented Oct 16, 2021

@rcurtin looks like some cert in the chain is outdated:

--2021-10-16 21:32:52--  https://mlpack.org/files/stb.tar.gz
Resolving mlpack.org (mlpack.org)... 161.8.32.26
Connecting to mlpack.org (mlpack.org)|161.8.32.26|:443... connected.
ERROR: cannot verify mlpack.org's certificate, issued by ‘CN=R3,O=Let's Encrypt,C=US’:

@zoq
Copy link
Member Author

zoq commented Oct 16, 2021

@mlpack-jenkins test this please

1 similar comment
@zoq
Copy link
Member Author

zoq commented Oct 16, 2021

@mlpack-jenkins test this please

@zoq
Copy link
Member Author

zoq commented Oct 17, 2021

@shrit does the cross-compiling expect that all deps are downloaded via the auto-downloader? When I run cmake inside the mlpack/jenkins-amd64-debian docker container it can find ensmallen but when I run cmake with the cross-compiling env enabled it can't find ensmallen anymore, the same applies to armadillo, but with 68cb6ee we download armadillo.

@shrit
Copy link
Member

shrit commented Oct 17, 2021

@zoq When cross-compiling we should only use the auto downloader to get the dependencies, we should not use the dependencies that are already installed on the machine.

@zoq
Copy link
Member Author

zoq commented Oct 17, 2021

What is the reason for this? ensmallen and stb is header only, so we could just use whatever is available. Or even if it isn't a user should be able to specify what to use. Maybe I'm missing something?

@shrit
Copy link
Member

shrit commented Oct 17, 2021

@zoq You have to download all dependencies when cross-compiling whether if they are header only or not, it does not matter. Header-only libraries make only the cross-compilation step easier and more comfortable since it removes the need to cross-compile the dependencies too.

The main reason for this is to avoid including the host system header. For instance, if you try to use ensmallen headers that are installed on the host system (x86), you will have to use this path /usr/include, this will include not only ensmallen library but the C++ std library that is designed and some part are compiled for the x86. As a result, you are telling the cross compiler that all the headers you need in order to compile for a target architecture (arm) can be found in /usr/include which is not the case since the target headers might be very different from the host headers (this vary and depends on the target architecture).

This is why we need a toolchain to cross-compile. Note that, It is true that a toolchain provides a cross-compiler, linkers, and all other compilations tools but it also provides the system roots, headers, and libraries that are unique for the target and they are different from the one installed on the host.

This is the trick about the cross-compilation, all the target (arm architecture in our case) headers and libraries need to be separated from the Host (x86 in our case) libraries and headers to avoid any possible mixes that can result in unsolvable issues.

That is why a lot of people consider cross-compilation as a nasty and painful thing to do. However, it is the only option we have when you need a lot of RAM to compile the project. Also, if you have an infrastructure that does that for you as we have done, with only a few things to compile, then there is nothing to worry about.

@zoq
Copy link
Member Author

zoq commented Oct 17, 2021

Thanks for the clarification, do you think another way would be to copy or symlink the files into the build/deps directory? A potential issue I see with the current approach is that you can't use your own package, like if someone uses a modified ensmallen lib or needs a specific version.

You are right that having a static system where we know exactly what packages are used is ideal since it removes dependency issues, so I don't like to change that; we just have to make sure this is documented somewhere because people might wonder why dependencies are downloaded that already exist on the host system.

@zoq zoq changed the title Disable auto-download by default Disable auto-download by default + improve test stability Oct 17, 2021
@zoq
Copy link
Member Author

zoq commented Oct 18, 2021

@Yashwants19 do you make any sense out of:

Run Rscript -e "rcmdcheck::rcmdcheck('mlpack_3.4.3.tar.gz', args = c('--no-manual','--as-cran'), error_on = 'warning', check_dir = 'check')"
Error: Error in loadNamespace(x) : there is no package called ‘rcmdcheck’
Calls: loadNamespace -> withRestarts -> withOneRestart -> doWithOneRestart

looks like the same command works on macOS and Windows.

@zoq
Copy link
Member Author

zoq commented Oct 22, 2021

@mlpack-jenkins test this please

@@ -64,7 +63,7 @@ jobs:
- name: CMake
run: |
mkdir build
cd build && cmake -DDEBUG=OFF -DPROFILE=OFF -DBUILD_CLI_EXECUTABLES=OFF -DBUILD_PYTHON_BINDINGS=OFF -DBUILD_JULIA_BINDINGS=OFF -DBUILD_GO_BINDINGS=OFF -DBUILD_R_BINDINGS=ON ..
cd build && cmake -DDEBUG=OFF -DPROFILE=OFF -DBUILD_CLI_EXECUTABLES=OFF -DBUILD_PYTHON_BINDINGS=OFF -DBUILD_JULIA_BINDINGS=OFF -DBUILD_GO_BINDINGS=OFF -DBUILD_R_BINDINGS=ON -DDOWNLOAD_DEPENDENCIES=ON ..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cd build && cmake -DDEBUG=OFF -DPROFILE=OFF -DBUILD_CLI_EXECUTABLES=OFF -DBUILD_PYTHON_BINDINGS=OFF -DBUILD_JULIA_BINDINGS=OFF -DBUILD_GO_BINDINGS=OFF -DBUILD_R_BINDINGS=ON -DDOWNLOAD_DEPENDENCIES=ON ..
cd build && pwd && cmake -G"Unix Makefiles" -DDEBUG=OFF -DPROFILE=OFF -DBUILD_CLI_EXECUTABLES=OFF -DBUILD_PYTHON_BINDINGS=OFF -DBUILD_JULIA_BINDINGS=OFF -DBUILD_GO_BINDINGS=OFF -DBUILD_R_BINDINGS=ON -DDOWNLOAD_DEPENDENCIES=ON ..

I read the log but don't really understand what happened. Can you try this? And maybe also add pwd and ls to the Build step below too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to remove the previously used armadillo build command.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I didn't look high enough up in the log :)

@zoq
Copy link
Member Author

zoq commented Oct 23, 2021

Fun, new issue on the windows build, at least the R build is fixed.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for putting so much time into the test fixes. Excited to see green builds again (minus the Windows builds I guess :)).

.ci/windows-steps.yaml Outdated Show resolved Hide resolved
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit db297e7 into mlpack:master Nov 2, 2021
This was referenced Oct 14, 2022
@rcurtin rcurtin mentioned this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants