-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXAPPS-581] Nightly Straight Dope tests. #11814
Conversation
ci/docker/runtime_functions.sh
Outdated
set -ex | ||
cd /work/mxnet/tests/nightly/straight_dope | ||
rm -rf ./straight_dope_book | ||
git clone https://github.com/zackchase/mxnet-the-straight-dope straight_dope_book |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to do this as part of the test class init? That way, the test would be self contained and doesn't need any bootstrapping.
93a9376
to
61e18c0
Compare
@marcoabreu Thanks for the suggestion. I've moved the notebook download to the installation steps. |
The Straight Dope notebooks will retrieved from the Github repo, run and scanned for warnings and errors. Because we are not checking accuracy of the training, we set the number of epochs to 1 to reduce the integration test run time. * Common functionality for running and testing notebooks has been factored into a common test util module. * Support for running UTF-8 notebooks added (Python2 and 3 compatible). * Notebooks requiring a single GPU and multi GPUs have been split into two different test suites so that they can be run on different hardware. * Add test to make sure that all notebooks are tested. * Comment out broken notebooks while they are being fixed (I will uncomment them in a follow up PR).
61e18c0
to
7e122bc
Compare
@@ -74,6 +74,10 @@ ARG GROUP_ID=0 | |||
COPY install/ubuntu_adduser.sh /work/ | |||
RUN /work/ubuntu_adduser.sh | |||
|
|||
# This needs to be run after /work/mxnet directory is created by ubuntu_adduser.sh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I mean that this should be done in https://docs.python.org/2/library/unittest.html#unittest.TestCase.setUpClass
If you do it here, you have two problems:
- The directory won't be valid since the docker build is independent from the runtime
- The result will be cached and thus we might not be able to detect problems in the straight dope up front
I know that my suggestion is not ideal, but otherwise we got a test don't won't be runnable if executed without proper knowledge about dependencies.
4565b15
to
688767f
Compare
Hi @marcoabreu, I've updated the test harness to clone the repo and removed the old code. I rebased so the changes are in: Please take a look. Thank you! Vishaal |
* Moving logic to download the Straight Dope notebooks to the test harness. * Remove cache logic as it is unnecessary.
688767f
to
bf50afd
Compare
cmd, | ||
stdout=subprocess.PIPE, | ||
stderr=subprocess.STDOUT) | ||
(out, _) = proc.communicate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Is there a way we could add a timeout here? Git tends to not be very nice to us in terms of reliability..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a commit to add a timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides timeout for git clone
I added a test job at http://jenkins.mxnet-ci.amazon-ml.com/job/test-straight-dope-nightly/. Please review it carefully. If everything passes, we're good to go.
403a6cb
to
244a1c0
Compare
244a1c0
to
cce0ca7
Compare
Thanks @marcoabreu , I added a timeout. Will keep tabs on the nightly test job you linked. |
Do you think it would make sense to enable the log output? Right now, the output is pretty empty and you can't really tell which chapters have run so far. |
416b2d9
to
4b9cb47
Compare
We're good to ship for a first initial version. I think you will then follow up with other PRs to activate the remaining tests and address further issues. |
Move two notebooks requiring multi-GPUs out of the single GPU test suite.
4b9cb47
to
47d0e08
Compare
Thanks for your help @marcoabreu Agreed! |
* [MXAPPS-581] Nightly Straight Dope tests. The Straight Dope notebooks will retrieved from the Github repo, run and scanned for warnings and errors. Because we are not checking accuracy of the training, we set the number of epochs to 1 to reduce the integration test run time. * Common functionality for running and testing notebooks has been factored into a common test util module. * Support for running UTF-8 notebooks added (Python2 and 3 compatible). * Notebooks requiring a single GPU and multi GPUs have been split into two different test suites so that they can be run on different hardware. * Add test to make sure that all notebooks are tested. * Comment out broken notebooks while they are being fixed (I will uncomment them in a follow up PR). * [MXAPPS-581] Download notebooks in test setup. * Moving logic to download the Straight Dope notebooks to the test harness. * Remove cache logic as it is unnecessary. * [MXAPPS-581] Add a timeout for download of notebooks. * [MXAPPS-581] Move notebooks requiring multi-gpus. Move two notebooks requiring multi-GPUs out of the single GPU test suite.
Description
The Straight Dope notebooks will retrieved from the Github repo, run and
scanned for warnings and errors. Because we are not checking accuracy of
the training, we set the number of epochs to 1 to reduce the integration
test run time.
factored into a common test util module.
into two different test suites so that they can be run on different
hardware.
uncomment them in a follow up PR).