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

[MXNET-1155] Add scala packageTest utility #13046

Merged
merged 4 commits into from
Dec 11, 2018
Merged

Conversation

zachgk
Copy link
Contributor

@zachgk zachgk commented Oct 30, 2018

Description

This PR creates a scala packageTest utility to run the full test suite against any mxnet jar. This can be used to verify that the packages produced for release and as part of the ci/cd run correctly on various environments (16.04, 18.04, centOS, amazon linux, etc.).

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@lanking520 @nswamy @andrewfayres @yzhliu

@ankkhedia
Copy link
Contributor

@zachgk Thanks for your contribution!
@mxnet-label-bot [ pr-awaiting-review, Scala ]

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review Scala labels Oct 30, 2018
Makefile Outdated
@@ -602,6 +602,14 @@ scalaclean:
(cd $(ROOTDIR)/scala-package; \
mvn clean -P$(SCALA_PKG_PROFILE),$(SCALA_VERSION_PROFILE))

scalatestcompile:
(cd $(ROOTDIR)/scala-package; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the parenthesis? Also what if cd fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly left the parenthesis because that is consistent with the rest of the scala make commands and we have been following that since 2452d87. They could all stand to be cleaned up, but that should be handled in a separate PR.

When you say cd fails, are you referring to the change directory or continuous deployment? Either way, is there something that should happen when it fails or some greater consequence of it failing? I'm not sure I get the question

Copy link
Contributor

Choose a reason for hiding this comment

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

you might want to use && instead of ; in case the previous command fails.

pllarroy@186590d670bd:0:~$ false ; echo keep going
keep going
pllarroy@186590d670bd:0:~$ false && echo keep going
pllarroy@186590d670bd:1:~$

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. That makes sense. I fixed it

@lanking520
Copy link
Member

One general question, is that possible to use one pom file to trigger all the tests instead of defining a scala test target in each subfolder?

@zachgk
Copy link
Contributor Author

zachgk commented Nov 1, 2018

The main reason I decided not to pursue using one pom file was because the tests can access local resources to their directory. For example, there are scripts used by the tests in scala-package/{core,examples}/scripts. In order for the packageTest tests to be able to access them, I had to create mirroring directories (symlink) in scala-package/packageTest/{core,examples}/scripts. Considering name conflicts as both folders are named scripts, I decided this was the easiest solution

@zachgk zachgk force-pushed the packageTest branch 2 times, most recently from 6d366b6 to a0bf25c Compare November 2, 2018 19:51
@stu1130
Copy link
Contributor

stu1130 commented Nov 21, 2018

@zachgk could you address the CI failure
@lanking520 @nswamy @andrewfayres @yzhliu ping for review

@zachgk zachgk force-pushed the packageTest branch 3 times, most recently from da17c93 to d6ad0e4 Compare November 29, 2018 03:32
Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, overall looks ok. Have some general questions: What will be the impact since you have placed test-jar generation in most of the code that contains test?

scala-package/packageTest/Makefile Show resolved Hide resolved
scala-package/packageTest/core/scripts Show resolved Hide resolved
Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

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

As discussed offline, please remove all redundant modification that would not be applied in here and only keep the things that are useful

scala-package/packageTest/README.md Show resolved Hide resolved
Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

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

Please address the problem.

@@ -600,61 +600,78 @@ rpkgtest:
Rscript -e 'res<-covr:::package_coverage("R-package");fileConn<-file(paste("r-package_coverage_",toString(runif(1)),".json"));writeLines(covr:::to_codecov(res), fileConn);close(fileConn)'

scalaclean:
(cd $(ROOTDIR)/scala-package; \
(cd $(ROOTDIR)/scala-package && \
Copy link
Member

Choose a reason for hiding this comment

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

why we use a && instead of ;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

&& executes the second command only if the first succeeds (see #13046 (comment))


### Test Installed Jars

If you have a jar file, you can install it to your maven cache repository(`~/.m2/repository`) using `mvn install:install-file -Dfile=<path-to-file>`. This might be useful if you acquire the .jar file from elsewhere. You can also run `make scalainstall` to install from a local build. Then, run `make testinstall` in the package test directory to run the tests. Note that unless you also install an additional mxnetexamples jar, you can only run the unit tests.
Copy link
Member

Choose a reason for hiding this comment

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

Currently as @frankfliu tested mvn install:install-file -Dfile=<path-to-file> seemed not functioning properly. Mac OSX crash and Linux Machine installed into a wrong jar. Please double check about this section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will modify the README instructions to specify either with pom or with all details

Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, LGTM!

java.lang.NoClassDefFoundError: org/apache/mxnetexamples/Util$
```

you are missing the mxnetexamples package. See your test mode installation section for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to the test mode installation section so that it is easy to navigate ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In document linking is not officially part of markdown. Github added the feature, but it won't work in other viewers such as intellij. I would rather have no link than a link that might not work

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay! Makes sense

Copy link
Contributor

@piyushghai piyushghai left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @zachgk !

@lanking520 lanking520 merged commit a4c97ec 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)
  ...
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Jan 3, 2019
* [MXNET-1155] Add scala packageTest utility

* Clean up utility

* Safe change directory in Makefile for scala

* mvn install file instructions with details
@zachgk zachgk deleted the packageTest branch April 12, 2019 22:27
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Apr 26, 2019
* [MXNET-1155] Add scala packageTest utility

* Clean up utility

* Safe change directory in Makefile for scala

* mvn install file instructions with details
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Apr 26, 2019
* [MXNET-1155] Add scala packageTest utility

* Clean up utility

* Safe change directory in Makefile for scala

* mvn install file instructions with details
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Apr 30, 2019
* [MXNET-1155] Add scala packageTest utility

* Clean up utility

* Safe change directory in Makefile for scala

* mvn install file instructions with details
zachgk added a commit to zachgk/incubator-mxnet that referenced this pull request May 16, 2019
* [MXNET-1155] Add scala packageTest utility

* Clean up utility

* Safe change directory in Makefile for scala

* mvn install file instructions with details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review Scala
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants