Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update build scripts to always run tests for all MXNet engine types #71

Open
wants to merge 6 commits into
base: v0.x.x
Choose a base branch
from
Open

Update build scripts to always run tests for all MXNet engine types #71

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0aa2462
Update build scripts to always run tests for all MXNet engine types
joseph-wakeling-sociomantic Jun 21, 2018
62e2506
Remove MXNET_ENGINE_TYPE settings from Travis CI config
joseph-wakeling-sociomantic Jun 21, 2018
201fe10
squash! Update build scripts to always run tests for all MXNet engine…
joseph-wakeling-sociomantic Jun 21, 2018
9370ffd
squash! Update build scripts to always run tests for all MXNet engine…
joseph-wakeling-sociomantic Jun 21, 2018
38c3e08
squash! Update build scripts to always run tests for all MXNet engine…
joseph-wakeling-sociomantic Jun 21, 2018
c8d44c2
squash! Update build scripts to always run tests for all MXNet engine…
joseph-wakeling-sociomantic Jun 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 4 additions & 21 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ env:
global:
# Make sure beaver is in the PATH
- PATH="$(git config -f .gitmodules submodule.beaver.path)/bin:$PATH)"
- BEAVER_DOCKER_VARS="MXNET_ENGINE_TYPE"

# Basic config is inherited from the global scope
jobs:
Expand All @@ -39,32 +38,16 @@ jobs:
include:
# Test matrix
- <<: *test-matrix
env: DMD=dmd1 DIST=xenial F=devel MXNET_ENGINE_TYPE=NaiveEngine
- <<: *test-matrix
env: DMD=dmd1 DIST=xenial F=devel MXNET_ENGINE_TYPE=ThreadedEngine
- <<: *test-matrix
env: DMD=dmd1 DIST=xenial F=devel MXNET_ENGINE_TYPE=ThreadedEnginePerDevice
env: DMD=dmd1 DIST=xenial F=devel

- <<: *test-matrix
env: DMD=dmd1 DIST=xenial F=production MXNET_ENGINE_TYPE=NaiveEngine
- <<: *test-matrix
env: DMD=dmd1 DIST=xenial F=production MXNET_ENGINE_TYPE=ThreadedEngine
- <<: *test-matrix
env: DMD=dmd1 DIST=xenial F=production MXNET_ENGINE_TYPE=ThreadedEnginePerDevice
env: DMD=dmd1 DIST=xenial F=production

- <<: *test-matrix
env: DMD=dmd-transitional DIST=xenial F=production MXNET_ENGINE_TYPE=NaiveEngine
- <<: *test-matrix
env: DMD=dmd-transitional DIST=xenial F=production MXNET_ENGINE_TYPE=ThreadedEngine
- <<: *test-matrix
env: DMD=dmd-transitional DIST=xenial F=production MXNET_ENGINE_TYPE=ThreadedEnginePerDevice
env: DMD=dmd-transitional DIST=xenial F=devel

- <<: *test-matrix
env: DMD=dmd-transitional DIST=xenial F=devel MXNET_ENGINE_TYPE=NaiveEngine
- <<: *test-matrix
env: DMD=dmd-transitional DIST=xenial F=devel MXNET_ENGINE_TYPE=ThreadedEngine
- <<: *test-matrix
env: DMD=dmd-transitional DIST=xenial F=devel MXNET_ENGINE_TYPE=ThreadedEnginePerDevice
env: DMD=dmd-transitional DIST=xenial F=production

# Additional stages

Expand Down
22 changes: 22 additions & 0 deletions Build.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,34 @@ endif
download-mnist: $C/script/download-mnist
$(call exec,sh $(if $V,,-x) $^,$(MNIST_DATA_DIR),$^)

# helper template to define targets for per-engine test runs
define run_test_with_engine
$(1).stamp: $(1)-$(2).stamp

$(1)-$(2).stamp: $1
$(call exec,MXNET_ENGINE_TYPE=$2 $1,$1,$2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you miss a $Vtouch $(1)-$(2).stamp here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, most likely. I think I tried using $@ and there was some objection to that (which I assume is about ordering of make's expansion of these tokens).

endef

# helper function to generate targets for per-engine test runs
test_with_engines = $(foreach engine,$(TEST_MXNET_ENGINES),\
$(eval $(call run_test_with_engine,$1,$(engine))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both eval and call seem necessary here too to avoid a missing separator error.


# extra build dependencies for integration tests
$O/test-mxnet: override LDFLAGS += -lz
$O/test-mxnet: override DFLAGS += -debug=MXNetHandleManualFree

# run integration tests with all specified engines
$(eval $(call test_with_engines,$O/test-mxnet))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way to avoid the $(eval $(call ... here? They both seem to be necessary, otherwise I get a missing separator error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. call is needed to run the function and eval is needed to evaluate the returned string describing the target (https://www.gnu.org/software/make/manual/html_node/Eval-Function.html#Eval-Function).


# extra runtime dependencies for integration tests
$O/test-mxnet.stamp: override ITFLAGS += $(MNIST_DATA_DIR)
$O/test-mxnet.stamp: download-mnist
$Vtouch $@ # override default implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tweak (and the similar one for unittests) is necessary to avoid the "normal" integration test run from also taking place. Would be nice to avoid if possible though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another approach is to have the MXNet default engine always executed and then change TEST_MXNET_ENGINES to mean additional engines to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It would probably be best in that case to remove the MXNET_ENGINE_TYPE ?= line from Config.mak as then we test the default case where the engine is not specified in the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would also avoid the need to override the default environment with lines like

$O/allunittests.stamp:
    $Vtouch $@

... so it's probably better this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HOWEVER, note that this then messes up things from another point of view, which is: what if I want to specify just one engine to test with, not as an extra?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously I can do MXNET_ENGINE_TYPE=MyEngine TEST_MXNET_ENGINES= make test all but this is getting finnicky. That's why from a CI perspective I'd rather have just one variable (the build script's variable) be the one the user has to care about.


$O/%unittests: override LDFLAGS += -lz

# run unittests with all specified engines
$(eval $(call test_with_engines,$O/allunittests))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor note: would it be worth killing the need to write $O here? i.e. have the calls be

$(eval $(call test_with_engines,allunittests)
$(eval $(call test_with_engines,test-mxnet)

... and do the $O stuff under the hood ... ?


$O/allunittests.stamp:
$Vtouch $@ # override default implementation
2 changes: 2 additions & 0 deletions Config.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
INTEGRATIONTEST := integrationtest

TEST_MXNET_ENGINES ?= NaiveEngine ThreadedEngine ThreadedEnginePerDevice
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One small note here: it should not be possible that the TEST_MXNET_ENGINES variable be empty. If I do:

TEST_MXNET_ENGINES= make test all

... then obviously, nothing runs. But there should be a failure in this case (which doesn't happen right now). Any thoughts on the best way to implement this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why there should be a failure. Because with empty TEST_MXNET_ENGINES you define no additional targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jens-mueller-sociomantic right now they are not ADDITIONAL targets. Of course, we could make it so. It just means that NaiveEngine tests will run twice. But if we kill the MXNET_ENGINE_TYPE ?= line in Config.mak, maybe that has sense (as we then test the case where MXNET_ENGINE_TYPE is not specified in the environment).

Copy link

Choose a reason for hiding this comment

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

Perhaps define the TEST_MXNET_ENGINES variable to some "always fails" phony target by default, requiring that user override it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nemanja-boric-sociomantic I'm not sure it really fixes the issue, because the problem is precisely if the user overrides the variable to set it to empty (a phony target won't solve that).

Copy link

Choose a reason for hiding this comment

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

Right, I was talking nonsense. Hm...

Copy link

Choose a reason for hiding this comment

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

There's a way of defining default to be NativeEngine and filtering it out from the TEST_MXNET_ENGINES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I think this is overthinking things. If someone sets the field to empty that's their problem ...


MXNET_ENGINE_TYPE ?= NaiveEngine
export MXNET_ENGINE_TYPE

Expand Down