[Tutorials] Fix wrong vetoing of tutorials #5938
Conversation
|
Starting build on |
|
Starting build on |
eguiraud
left a comment
eguiraud
left a comment
There was a problem hiding this comment.
Looks good to me, I cannot approve just because I am not sure that our policy w.r.t. the pandas and numba tutorials is to deactivate them at configure time if the package is not there. At some point there was a discussion and I think the outcome was to always keep these tutorials in and install pandas and numba on all build nodes. I might be misremembering or might have missed further developments.
So I'd ask @Axel-Naumann or @stwunsch to give a final approval
That's fair, but users who don't have either of them installed will get broken tests if they choose to run the tests. |
|
Between silently not running tutorials (like we were doing until this PR, thank you very much for spotting and fixing it) and leaving those very rare users that run our full test suite with a few red python tutorials/tests (with error messages that say "please install pandas/numba"), I'd say the latter is preferable 😄 There might be a third option I don't see at the moment. In any case, I'm leaving the approval in the hands of who knows better than me :D |
|
Starting build on |
Well ... for what it is worth, I am one those users ... (and a minimal build would also be one of those users, wouldn't it be?). If the ROOT build configure successfully and build successfully, then ctest should be clean also. I.e. roottest failing because numba is not installed would, in my opinion, means that the cmake configure step of ROOT should fail if numba is not installed ... i.e. numba would become a hard dependency. |
|
Let's not rehash the discussion some of us already had (with the exact same arguments), and ideally not in writing on GitHub. Instead it might be more efficient to discuss this at a team meeting. I won't be able to attend on Monday; I suggest we discuss this in nine days. |
|
Starting build on |
I agree (hence this PR), but I think I have found a solution (@Axel-Naumann). Look at this: CMake's |
|
Starting build on |
|
Build failed on ROOT-performance-centos7-multicore/default. Errors:
Failing tests: |
|
Starting build on |
|
Build failed on windows10/cxx14. |
|
Build failed on ROOT-performance-centos7-multicore/default. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on ROOT-fedora30/cxx14. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
Build failed on ROOT-performance-centos7-multicore/default. Failing tests: |
|
Build failed on mac1015/cxx17. Failing tests: |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on mac1014/python3. Failing tests: |
|
Starting build on |
|
Build failed on ROOT-performance-centos7-multicore/default. Errors:
Failing tests: |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on mac1015/cxx17. Failing tests: |
- python tutorials were only globbed using tutorials/*/*py, so some
missed. Now recursive globbing is applied.
- Vetoing tutorials, e.g. when components are not active, was done in
two places, one for C++ and one for python. Now, blocks such as
if(NOT dataframe)
..
endif()
take care of both the python and C++ vetos in a single location.
This reduces the chance of forgetting to veto/enable some tutorials.
- CMake now correctly reports how many tutorials have been activated,
both for python and C++.
- For an unknown reason, tmva was vetoing both tmva and dataframe
python tutorials if xgboost was not found. In this commit, this is
fixed, and the veto code becomes more readable.
- Since keras is not a "required" dependency, the keras tutorials remain
deactivated if it's not found. Previously, they were never run, anyway.
If a test requires a specific python packge, this can now (and should!) be communicated using the argument PYTHON_DEPS when registering the test. This will trigger the addition of a test fixture, which checks for this package. This fixture is a simple ctest that tries to import the requested package. If the fixture passes, all dependent tests run. If the fixture fails, the dependent test don't run, but are all marked as failed because of the missing package. This makes tests faster and failures easier to diagnose, because it becomes obvious that those tests failed because of the missing package, and not because there's another problem in the test. Further, the tests that have python runtime dependencies will automatically be labelled with "python_runtime_deps".
Add numpy and xgboost.
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
And 19 more |
|
Build failed on mac1014/python3. Failing tests:
|
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests:
|
[ROOT-10921] Until pandas is installed on the CI nodes, df026 needs to stay disabled.
|
Starting build on |
oshadura
left a comment
oshadura
left a comment
There was a problem hiding this comment.
Just wow! Many thanks, you did amazing job!
Follows up on root-project#5938, adding support for the `PYTHON_DEPS` argument also to the `ROOTTEST_ADD_TEST` macro. Use this functionality to disable JupyROOT tests if the relevant libraries are not available.
Follows up on #5938, adding support for the `PYTHON_DEPS` argument also to the `ROOTTEST_ADD_TEST` macro. Use this functionality to disable JupyROOT tests if the relevant libraries are not available.
Follows up on root-project#5938, adding support for the `PYTHON_DEPS` argument also to the `ROOTTEST_ADD_TEST` macro. Use this functionality to disable JupyROOT tests if the relevant libraries are not available.
7 participants
This depends on #5998
Overhaul of veto-or-fail when python packages are missing
Disable (instead of veto) python tutorials that cannot work when python packages are missing. This happens by listing the required packages when registering the test or tutorial:
Before launching the tutorial/test, cmake will run a fixture, which is a simple
import <package>. If this passes, all dependent tutorials/tests are run. If it fails, it looks like this:The advantage of this is that one doesn't even have to open the test output, because cmake already communicates clearly that the failure is due to missing packages.
As discussed in Moday's meeting, all tests with extra runtime dependencies now are also labelled
python_runtime_deps. This gets assigned automatically ifPYTHON_DEPSis not empty.