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 all 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
42 changes: 40 additions & 2 deletions Build.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,50 @@ 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
#
# Params:
# $1 = path to executable that runs tests
# $2 = name of MXNet engine to use
define test_with_engine
$(1).stamp: $(1)-$(2).stamp

$(1)-$(2).stamp: $1
$(call exec,MXNET_ENGINE_TYPE=$2 $1,$1,$2)
$Vtouch $$@
endef

# helper function to generate targets for per-engine test runs
#
# Params:
# $1 = path to executable that runs tests
define run_test_with_engines
$(foreach engine,$(TEST_MXNET_ENGINES),\
$(eval $(call test_with_engine,$1,$(engine))))

$(1).stamp:
$Vtouch $$@ # override default implementation
endef

define run_test_with_dependency
$(foreach engine,$(TEST_MXNET_ENGINES),\
$(eval $(1)-$(engine).stamp: $2))
endef

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

# extra runtime dependencies for integration tests
$O/test-mxnet.stamp: override ITFLAGS += $(MNIST_DATA_DIR)
$O/test-mxnet.stamp: download-mnist
$(eval $(call run_test_with_dependency,$O/test-mxnet,\
override ITFLAGS += $(MNIST_DATA_DIR)))
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'm not convinced that this is working. The output of make -p gives e.g.:

build/devel/tmp/test-mxnet-NaiveEngine.stamp: ITFLAGS += ~/data/mnist

(where ~ is expanded;-) but the integration tests (and download script) both use the default value of this parameter, despite it being set in my Config.local.mak.

That said, this might be something other than how this Build.mak is written.

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've just validated with current v0.x.x, where things are working w.r.t. running test-mxnet, but not in terms of where the download script places things (which appears to be an existing Build.mak bug). So there seems to be some problem with how this override is being handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it's because the custom rule implemented in test_with_engine is missing the details used in the default makd integration test run:

# General rule to Run the test suite binaries
$O/test-%.stamp: $O/test-%
    $(call exec,$< $(_test_drt_opts) $(ITFLAGS),$<,run)
    $Vtouch $@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, I can easily add $$(_test_drt_opts) $$(ITFLAGS) to the call implemented here in Build.mak, but note that this is supposed to work for unittests as well as integration tests and there are no similar settings in vanilla makd where unittests are concerned.

@leandro-lucarella-sociomantic @nemanja-boric-sociomantic any advice/thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_test_drt_opts are common, I think, but unittests versus integration-tests should use UTFLAGS versus ITFLAGS respectively. I guess I could define a function to work out (based on the full target name) which options should be used. But it really feels like this is starting to turn into a nasty code duplication between how this Build.mak is doing things, versus how makd is doing things.

Copy link

Choose a reason for hiding this comment

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

TBH, my feeling is you have reached the level of the complexity where we should consider supporting this as a makd feature. @leandro-lucarella-sociomantic

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, I agree, this looks like it needs makd support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question is what level of makd support, though. It's not clear to me that it needs to be at the level of actually supporting the feature needed here (i.e. running tests multiple times for different environment settings). It might suffice to provide make functions to run the tests associated with a particular .stamp target (so that create a custom recipe without having to do a complete re-duplication of what makd already does).

@leandro-lucarella-sociomantic any thoughts? (Including "Drop this for now and get on with more actionable items" :-P)

Copy link

Choose a reason for hiding this comment

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

@leandro-lucarella-sociomantic your input is needed here :-)

$(eval $(call run_test_with_dependency,$O/test-mxnet,\
download-mnist))

# run integration tests with all specified engines
$(eval $(call run_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.

A weird behaviour: right now make test always re-runs the integration tests, despite the associated .stamp files being correctly generated. Any idea what could be responsible?

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, it looks like the download-mnist phony target is responsible, since there is no associated .stamp. It's independent of this PR, just more noticeable.


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

# run unittests with all specified engines
$(eval $(call run_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.

OTOH this works just fine, with no re-running of unittests after they have been successfully run once.

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