[Test] [Example] adding tests for examples #13271

ankkhedia · 2018-11-14T22:24:25Z

Description

This PR adds a test framework for examples.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Add a placeholder to test examples.

ankkhedia · 2018-11-14T22:24:59Z

@mxnet-label-bot add [pr-work-in-progress, Example]

roywei

Thanks for starting this! It will help detecting broken examples.
How about first try out a simple one for 5 epoch before adding more examples.
We need to see if it's correctly triggered in nightly tests

roywei · 2018-11-15T00:00:11Z

tests/examples/test_examples.py

+            return False
+        return True
+
+def test_cifar():


For our first test, maybe start with train_mnis with mlp network for 5 epochs?

anirudhacharya · 2018-11-15T08:18:41Z

tests/examples/test_examples.py

+    """
+    errors = []
+    try:
+    	check_call(command)


check_call -> subprocess.check_call

This code is not being used or run anywhere. Not getting tested either. I would prefer if these changes and PR - #13270 were merged into a single PR.

The above comment is just one instance of an error that might go unnoticed if we have separate PRs.

The PR will go together. Its still a WIP and has not been tested completely. It will be tested before merging

anirudhacharya · 2018-11-15T08:28:32Z

tests/examples/test_examples.py

+    	check_call(command)
+    except Exception as err:
+        err_msg = str(err)
+        errors.append(err_msg)


you could combine these two statements - errors.append(str(err_msg))

anirudhacharya · 2018-11-15T08:36:45Z

tests/examples/test_examples.py

+        err_msg = str(err)
+        errors.append(err_msg)
+    finally:
+        if len(errors) > 0:


a more pythonic suggestion if errors:

anirudhacharya · 2018-11-15T08:38:25Z

tests/examples/test_examples.py

+        errors.append(err_msg)
+    finally:
+        if len(errors) > 0:
+            logging.error('\n'.join(errors))


why is this happening in the finally block. Why not just log it in the except block and return false. And remove the finally block.

anirudhacharya · 2018-11-15T08:46:38Z

tests/examples/test_examples.py

+    errors = []
+    try:
+    	check_call(command)
+    except Exception as err:


what sort of exception are we expecting here? I think a CalledProcess(Exception/Error).

Better to know what exception we are catching than using a generic Exception to catch all errors.

If any exception gets caught it means a failure of example run so I think it makes sense to catch all the exceptions

anirudhacharya · 2018-11-15T09:02:24Z

tests/examples/test_examples.py

+
+#pylint: disable=no-member, too-many-locals, too-many-branches, no-self-use, broad-except, lost-exception, too-many-nested-blocks, too-few-public-methods, invalid-name
+"""
+    This file tests and ensures that the examples run correctly.


it does not ensure the correctness of the example. it only performs a sanity check of the example. can you please change this.

The discussion on the dev-list suggest that we start small just to check if examples are running fine. The individual examples test will have to take care of what correctness atrribute they want to check and should be taken care by the test owners. This PR just provide a template for putting tests for examples. The test owners are free to have their own helper functions and tests which can be generalised later. As we see the tests because some examples do inference, some do training and everything has a different form of output , it cannot be generalised at this point.

Please feel free to add additional helper functions over here if you feel it can be used across multiple examples.

I will change the wordings here that the examples run without errors if that makes more sense

ankkhedia · 2018-11-16T01:06:00Z

@marcoabreu @kalyc @roywei @nswamy
Could you please take a look . This PR just defines basic structure and common functionalities can be abstracted out for reuse as we add more tests.

@marcoabreu Also we would require help for test in CI setup.

kalyc

Has this file been tested locally?

kalyc · 2018-11-16T21:47:51Z

tests/examples/test_examples.py

+
+    Returns
+    -------
+        True if there are no warnings or errors.


[minor] unindent

kalyc · 2018-11-16T21:50:25Z

tests/examples/test_examples.py

+    if not os.path.isdir(working_dir):
+        os.makedirs(working_dir)
+        os.chdir(working_dir)
+    assert _run_command(example_name , ['python',os.path.join(example_dir,'train_cifar10.py')])


add condition that is being tested here -
assert True, _run_command(example_name , ['python',os.path.join(example_dir,'train_cifar10.py')])

Could you elaborate? I'm having trouble understanding what exactly you mean

@marcoabreu Could you please elaborate if this is for the code or comment?

I meant something like this https://github.com/apache/incubator-mxnet/blob/master/tests/tutorials/test_sanity_tutorials.py#L97 - but noticed that True has been asserted without explicitly specifying the condition - https://github.com/apache/incubator-mxnet/blob/master/tests/tutorials/test_tutorials.py#L62
So you can keep the assertion the way it is, or add True for increasing readability @ankkhedia

Ah I see :) in python you generally omit booleans during comparisons.
But what's important here is that actually the return value is being evaluated. If the test succeeded, it's true, otherwise it's false. Your example is for an exception case, which has been handled a bit differently in this approach

I am going with Marco's suggestion on this :)

marcoabreu · 2018-11-16T21:55:48Z

@jlcontreras please assist with the CI setup

marcoabreu · 2018-11-16T21:57:28Z

tests/examples/test_examples.py

+
+def test_cifar_default():
+    example_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'examples','image_classification')
+    temp_dir = 'tmpdir'


https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory

@marcoabreu The teamp file gets created in some other folder structure whereas I want a temp file within same folder as there are some data and other artifacts required to run the example in this folder.

Are the paths in the examples hardcoded or what's the issue here? Could you point me to some links?

The path inside example scripts are hardcoded.
https://github.com/apache/incubator-mxnet/blob/master/example/image-classification/train_cifar10.py#L26

roywei

You can test this script locally using `nosetests test_examples.py'

For me adding the following test works, I don't need to create or delete tmp directories as these two tests don't generate checkpoint files.


def test_mnist_mlp():
    assert _run_command('mnist_mlp' , ['python', 'example/image-classification/train_mnist.py', '--num-epochs', '5'])

def test_mnist_resnet():
    assert _run_command('mnist_resnet' , ['python', 'example/image-classification/train_mnist.py', '--network','resnet','--num-layers','110', '--gpus', '0', --num-epochs', '1'])

roywei · 2018-11-16T23:36:41Z

tests/examples/test_examples.py

+    example_dir = os.path.join(os.path.dirname(__file__), '..', '..', 'examples','image_classification')
+    temp_dir = 'tmpdir'
+    example_name = 'test_cifar10'
+    working_dir = os.path.join(*([temp_dir] + example_name)


missing ) here?

sync

ankkhedia · 2018-11-20T01:29:44Z

@jlcontreras @marcoabreu Could someone help us with the CI runs ?

piyushghai · 2018-11-20T19:22:09Z

tests/examples/test_examples.py

+    shutil.rmtree(temp_dir, ignore_errors=True)
+    if not os.path.isdir(working_dir):
+        os.makedirs(working_dir)
+        os.chdir(working_dir)


Should this chdir be outside the if check ?

Thanks for pointing out

ankkhedia · 2018-11-21T22:57:54Z

@kalyc The file has been tested locally

ankkhedia · 2018-11-21T23:01:36Z

@mxnet-label-bot add [pr-awaiting-review]
@mxnet-label-bot remove [pr-work-in-progress]

kalyc · 2018-11-21T23:05:21Z

@mxnet-label-bot update [pr-awaiting-review]

vandanavk · 2018-11-27T21:11:37Z

LGTM.

@mxnet-label-bot update [pr-awaiting-testing]

@Chancebair for review/help with the CI

vandanavk · 2018-12-01T00:09:47Z

@marcoabreu @Chancebair @jlcontreras could you help with the CI for this PR?

roywei · 2018-12-11T01:52:10Z

Can we fix the CI failure?

kalyc · 2018-12-18T21:25:36Z

As discussed offline we should merge the PR into the jenkins setup for testing examples to be able to test here - http://jenkins.mxnet-ci-dev.amazon-ml.com/job/test-kalyc-NightlyTestsForBinaries/job/jenkins_setup_testing_examples/

@ankkhedia please close the PR I will pull your changes here #13270

kalyc · 2018-12-18T23:31:27Z

Updated PR #13270 with your changes @ankkhedia
Please feel free to close this one.

ankkhedia · 2018-12-19T17:52:47Z

@kalyc Thanks for taking it forward.

adding tests for examples

13b27ee

marcoabreu added Example pr-work-in-progress PR is still work in progress labels Nov 14, 2018

roywei reviewed Nov 15, 2018

View reviewed changes

roywei mentioned this pull request Nov 15, 2018

Add Jenkins setup for running nightly tests on examples #13270

Closed

6 tasks

anirudhacharya suggested changes Nov 15, 2018

View reviewed changes

roywei mentioned this pull request Nov 15, 2018

[Example] fix train mnist for inception-bn and resnet #13239

Merged

5 tasks

ankkhedia added 3 commits November 15, 2018 16:07

addressing comments

d8d27b0

addressing comments

c3f74cf

addressing comments

aeb0811

ankkhedia changed the title ~~[WIP] [Example] adding tests for examples~~ [Test] [Example] adding tests for examples Nov 16, 2018

kalyc reviewed Nov 16, 2018

View reviewed changes

marcoabreu reviewed Nov 16, 2018

View reviewed changes

roywei reviewed Nov 16, 2018

View reviewed changes

ankkhedia added 3 commits November 19, 2018 10:41

addressed few review comments

a2d5551

Merge branch 'master' into example_ci

0250cf5

sync

addressing comments

c61ddd8

addressing comments

aee0f50

piyushghai reviewed Nov 20, 2018

View reviewed changes

fixing chdir

83fc00b

marcoabreu added pr-awaiting-review PR is waiting for code review and removed Example pr-work-in-progress PR is still work in progress labels Nov 21, 2018

marcoabreu added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Nov 27, 2018

ankkhedia closed this Dec 19, 2018

[Test] [Example] adding tests for examples #13271

[Test] [Example] adding tests for examples #13271

Uh oh!

Conversation

ankkhedia commented Nov 14, 2018

Description

Checklist

Essentials

Changes

Uh oh!

ankkhedia commented Nov 14, 2018

Uh oh!

roywei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankkhedia Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankkhedia Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankkhedia Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankkhedia commented Nov 16, 2018

Uh oh!

kalyc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu commented Nov 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankkhedia Nov 16, 2018 •

edited

Loading

ankkhedia Nov 15, 2018 •

edited

Loading

ankkhedia Nov 15, 2018 •

edited

Loading

marcoabreu Nov 16, 2018 •

edited

Loading

piyushghai Nov 20, 2018 •

edited

Loading

ankkhedia commented Nov 21, 2018 •

edited

Loading