Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

#13385 [Clojure] - Turn examples into integration tests #13554

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

hellonico
Copy link
Contributor

!!! This is work in progress.

Would:

  • the committed script
  • adding a bare minimum deftest inside each example

be ok?

@gigasquid
Copy link
Member

Thanks so much @hellonico for tackling this. This will be a big improvement to stability and regression of the package as we develop it. The way you are approaching it with a minimum test and then the script seems like the right direction. 😸

@gigasquid gigasquid added the pr-work-in-progress PR is still work in progress label Dec 6, 2018
@hellonico
Copy link
Contributor Author

The following examples are turned into integration tests:

SuperPinkicious:examples niko$ find . -name test
./gan/test
./visualization/test
./neural-style/test
./tutorial/test
./multi-label/test
./module/test
./cnn-text-classification/test
./imclassification/test
./profiler/test
./rnn/test

rnn has a training section and a test trained network section, but the training section is very, very, very slow
I am wondering if it should still be added or not? It largely depends on the CI setup at this stage, but on my dev machine I can not get anything in a reasonable amount of time.

@gigasquid
Copy link
Member

gigasquid commented Dec 8, 2018

@hellonico - Awesome work!
Regarding the rnn example - I think limiting it to just the test trained network is sufficient and will keep the time down to a somewhat reasonable level. I would like to be able to run the suite locally as well as CI.

@hellonico
Copy link
Contributor Author

Good morning !
Great. Just squashed the commits ... and rebased on master.

@gigasquid
Copy link
Member

gigasquid commented Dec 9, 2018 via email

@hellonico hellonico force-pushed the topic/13385 branch 10 times, most recently from e6f0888 to af4706b Compare December 10, 2018 05:31
@hellonico
Copy link
Contributor Author

ci/jenkins/mxnet-validation/sanity — Job failed

Any idea on how to avoid the (de facto not very possible) license check on json files ?

@hellonico
Copy link
Contributor Author

hellonico commented Dec 10, 2018

Checked on OSX, and on two linux boxes:
manjaro.txt
ubuntu.txt

Finally ...

@gigasquid
Copy link
Member

Awesome work! I think we can get the CI to pass the RAT check if you put the files having trouble:

mxnet/contrib/clojure-package/examples/imclassification/test/test-symbol.json.ref
mxnet/contrib/clojure-package/examples/profiler/test/profile-matmul-20iter.json.ref

in here https://github.com/apache/incubator-mxnet/blob/master/tests/nightly/apache_rat_license_check/rat-excludes

It gets called from here https://github.com/apache/incubator-mxnet/blob/master/ci/docker/runtime_functions.sh#L996

@marcoabreu Do you have thoughts on how these integration tests should be integrated in the CI? Should it be nightly since they run pretty long? Should we merge this first and then open another one for integrating into the CI?

@marcoabreu
Copy link
Contributor

Yeah nightly would be a good place. I'll leave it up to you whether we do it in this PR or a separate one. But I think a separate one is a good idea

@hellonico
Copy link
Contributor Author

hellonico commented Dec 11, 2018

  • Rebased master (monday 10th commits)
  • Updated json files to be ignored in the nightly checks

Ready to be merged !

Let's find out how much time it takes to run the suite. When the models are already there, those tests run in about 10-15 minutes on a slow machine.

@gigasquid
Copy link
Member

Thanks for taking this on and making it happen 💯

I'm going to go ahead a merge this and then we can work on adding it to the nightly tests in another PR and see how it runs there.

@gigasquid gigasquid merged commit 449e17d into apache:master Dec 11, 2018
zhaoyao73 added a commit to zhaoyao73/incubator-mxnet that referenced this pull request Dec 13, 2018
* upstream/master: (38 commits)
  Feature/mkldnn static (apache#13628)
  Fix the bug of BidirectionalCell (apache#13575)
  Set install path for libmxnet.so dynamic lib on Mac OS (apache#13629)
  add batch norm test (apache#13625)
  Scripts for building dependency libraries of MXNet (apache#13282)
  fix quantize pass error when the quantization supported Op are excluded in the model (apache#13596)
  Optimize C++ API (apache#13496)
  Fix warning in waitall doc (apache#13618)
  [MXNET-1225] Always use config.mk in make install instructions (apache#13364)
  [MXNET-1224]: improve scala maven jni build and packing. (apache#13493)
  [MXNET-1155] Add scala packageTest utility (apache#13046)
  fix the Float not showing correctly problem (apache#13617)
  apache#13385 [Clojure] - Turn examples into integration tests (apache#13554)
  Add Intel MKL blas to Jenkins (apache#13607)
  Revert "[MXNET-1198] MXNet Java API (apache#13162)"
  Reducing the length of setup tutorial (apache#13306)
  [MXNET-1182] Predictor example (apache#13237)
  [MXNET-1187] Added Java SSD Inference Tutorial for website (apache#13201)
  add defaults and clean up the tests (apache#13295)
  [MXNET-1181] Added command line alternative to IntelliJ in install instructions (apache#13267)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Clojure pr-work-in-progress PR is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants