[MXNET-531] Integration Test for Scala #11596

lanking520 · 2018-07-07T01:48:50Z

Description

This is an approach for us to do integration test on Scala. As more of the examples are adding in the Scala CI, we need a separation between tests to make sure it test different things. However, this solution is not perfect due to this fact:

I have placed verify stage on the example module and change the unit test to integration test. If we run the verify stage, the unit test will still be run though it takes us 1 ~ 2 minutes to finish them.

@nswamy @marcoabreu @yzhliu @andrewfayres

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

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

lanking520 · 2018-07-09T05:25:35Z

@marcoabreu I didn't see the integration test section in our CI pipeline. Have we merged them together in 1 test phase?

nswamy · 2018-07-09T20:31:29Z

Makefile

+			-Dcflags="$(CFLAGS)" -Dldflags="$(LDFLAGS)" \
+			-Dlddeps="$(LIB_DEP) $(ROOTDIR)/lib/libmxnet.a" $(SCALA_TEST_ARGS))
+
+scalaintegrationtest:


where do you specify to run tests only under examples ? does it also run unit tests?

Currently, all unit test are run on integration-test stage in Maven and all integration test runs in verify stage.

Yes, it runs unit test as well

I am not sure I see the benefit of creating another stage that runs twice. Am I missing anything ? consider adding the test phase in examples under a maven profile which can passed when you call make integrationtests ?

It works like this way:
if you put test in the test phase, test will be running in test, package and all after phases.
If you put test in the integration-test, test will be running in verify, but not in package.
So it is impossible to keep test runs on a individual phase.

Some alternative is applicable for tests done in Java. However we are using scalatest plugin, the plugins for Java surefire cannot used in here.

Can you not run verify of examples under a called profile integration-test

Now there is no verify. Only Integration test.

I tried to centralize the two profiles in the main pom file instead of changing these properties in every module. The test plugins are spread in all the modules, After cleaning them and adding two profiles in the main pom configuration. The tests are now segfaulting, not sure what is causing. I will stop this effort here and we can see later.

lanking520 · 2018-07-10T00:48:01Z

I have make the best use of profile and I think it should works fine without duplicated tests. There is also changes on the build pipeline, please let me know your idea. @nswamy @yzhliu

yzhliu · 2018-07-10T17:15:55Z

good to me

yzhliu · 2018-07-11T18:20:48Z

Makefile

@@ -593,9 +593,15 @@ scalapkg:
 			-Dcurrent_libdir="$(ROOTDIR)/lib" \
 			-Dlddeps="$(LIB_DEP) $(ROOTDIR)/lib/libmxnet.a")

-scalatest:
+scalaunittest:


remember to update README

Changed the README in the scala package.

I didn't notice. my bad..

marcoabreu · 2018-07-11T18:30:20Z

Yes, the Scala integration tests are now in parallel with the unit tests. At some point we might move them to nightly, but for now, you don't have to worry but just categorize the tests properly.

nswamy · 2018-07-12T05:17:29Z

@lanking520 thanks for separating the tests into two phases, this definitely helps in speeding up the CI pipeline.
@marcoabreu consider moving Scala Integration tests to the next stage, we don't need to run Unit Tests and Integration tests in parallel

marcoabreu · 2018-07-12T08:53:43Z

Jenkinsfile

@@ -712,18 +712,6 @@ try {
        }
      }
    },
-    'Scala: GPU': {


@nswamy @lanking520 was this intentional? I think you just disabled all unit tests for Scala on GPU ._.

marcoabreu · 2018-07-12T08:54:39Z

Jenkinsfile

@@ -979,6 +967,18 @@ try {
          }
        }
      }
+    },
+    'Scala: GPU': {


Please name this Scala Integration: GPU

marcoabreu · 2018-07-12T08:56:33Z

Could you elaborate, naveen? When running integration tests, does it automatically cover unit tests as well? I'm afraid merging them will

Increase the time of the critical path
Make it hard to distinguish between unittests and integration tests later on - right now it looks like we're not running unit tests at all

lanking520 · 2018-07-12T16:39:45Z

@marcoabreu You are right, the CPU should be called as Scala Unit test CPU. Currently, we do not have any unit test running on GPU. Only some tests in integration test run GPU. I will amend with another PR to add this fixes.

* Separate unit test and integration tests for Scala

lanking520 requested review from marcoabreu, nswamy, szha and yzhliu as code owners July 7, 2018 01:48

lanking520 force-pushed the integration-ci branch from 4c89fda to 6ef39a7 Compare July 8, 2018 06:05

yzhliu approved these changes Jul 9, 2018

View reviewed changes

nswamy reviewed Jul 9, 2018

View reviewed changes

lanking520 force-pushed the integration-ci branch from 6ef39a7 to 034308a Compare July 10, 2018 00:41

lanking520 force-pushed the integration-ci branch from 034308a to ffdca8b Compare July 10, 2018 17:38

nswamy mentioned this pull request Jul 11, 2018

Updating MultiTask example to use new infer api and adding test for CI #11605

Merged

7 tasks

yzhliu suggested changes Jul 11, 2018

View reviewed changes

lanking520 added 6 commits July 11, 2018 13:24

add unittest and integrationtest for scala

9eca61b

add CI support

4d9ac2d

fix semi block

abd3033

alternative solution -- profile

b0a98b8

revert the module change section

296487e

update with readme

cd3017a

lanking520 force-pushed the integration-ci branch from ffdca8b to cd3017a Compare July 11, 2018 20:24

yzhliu approved these changes Jul 12, 2018

View reviewed changes

nswamy merged commit b786ead into apache:master Jul 12, 2018

marcoabreu reviewed Jul 12, 2018

View reviewed changes

Jenkinsfile

@@ -979,6 +967,18 @@ try {

}

}

}

},

'Scala: GPU': {

Copy link

Contributor

marcoabreu Jul 12, 2018

Choose a reason for hiding this comment

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

Please name this Scala Integration: GPU

lanking520 deleted the integration-ci branch July 17, 2018 21:57

XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018

[MXNET-531] Integration Test for Scala (apache#11596)

0caf4e4

* Separate unit test and integration tests for Scala

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-531] Integration Test for Scala #11596

[MXNET-531] Integration Test for Scala #11596

lanking520 commented Jul 7, 2018

lanking520 commented Jul 9, 2018

nswamy Jul 9, 2018

lanking520 Jul 9, 2018

nswamy Jul 9, 2018

lanking520 Jul 9, 2018

lanking520 Jul 9, 2018

nswamy Jul 10, 2018

lanking520 Jul 10, 2018

nswamy Jul 12, 2018

lanking520 commented Jul 10, 2018

yzhliu commented Jul 10, 2018

yzhliu Jul 11, 2018

lanking520 Jul 11, 2018

yzhliu Jul 12, 2018

marcoabreu commented Jul 11, 2018

nswamy commented Jul 12, 2018

marcoabreu Jul 12, 2018

marcoabreu Jul 12, 2018

marcoabreu commented Jul 12, 2018

lanking520 commented Jul 12, 2018

[MXNET-531] Integration Test for Scala #11596

[MXNET-531] Integration Test for Scala #11596

Conversation

lanking520 commented Jul 7, 2018

Description

Checklist

Essentials

Changes

Comments

lanking520 commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanking520 commented Jul 10, 2018

yzhliu commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu commented Jul 11, 2018

nswamy commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu commented Jul 12, 2018

lanking520 commented Jul 12, 2018