Update build scripts to always run tests for all MXNet engine types #71
Conversation
This patch updates `Build.mak` to ensure that unittests and integration tests will always be run for all of the supported engine types. As well as making it easier to run tests for all engines locally, this will make it possible to greatly simplify our CI setup.
|
I'm going to follow up with a separate travis config patch. For now I want to see the behavioural difference without it ;-) |
Since `Build.mak` now takes care of running tests with all engines, we can greatly simplify the number and configuration of Travis CI jobs. This patch reduces the setup to cover development and production builds for D1 and D2 respectively.
Build.mak
Outdated
| $O/test-mxnet.stamp: $O/test-mxnet download-mnist | ||
| $(call run_test,$<,NaiveEngine) | ||
| $(call run_test,$<,ThreadedEngine) | ||
| $(call run_test,$<,ThreadedEnginePerDevice) |
There was a problem hiding this comment.
It occurs to me that an even better option would be to have a variable defining the supported engines (set in Config.mak if not already defined) and just foreach over the entries in that variable. But I'm finding it tricky to work out how best to do that :-\
There was a problem hiding this comment.
Specifically: how to implement an appropriate for loop. Although I guess a simple, manual way is to have a separate run_test_with_all_engines function which takes a single parameter (the test to run).
There was a problem hiding this comment.
I've tried with:
run_test_with_all_engines = $(foreach engine,$(SUPPORTED_MXNET_ENGINES),$(call run_test,$1,$(engine)))
... but for some reason, this only ever picks up on the first entry in the SUPPORTED_MXNET_ENGINES variable.
There was a problem hiding this comment.
BTW, the dependency of $O/test-mxnet is added automatically by Makd, you don't need to put it explicitly.
There was a problem hiding this comment.
This is what I assumed, but it didn't work without it. :-\
|
I've pushed a follow-up patch to simplify the Travis config. |
|
(I would also squash) |
Agreed -- it's what I was trying to do by defining a variable defining the engines to test. Do you have any suggestions for how to do the kind of foreach loop I had in mind above? |
|
I've pushed a |
|
... and one more minor tweak. |
Build.mak
Outdated
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Another approach is to have the MXNet default engine always executed and then change TEST_MXNET_ENGINES to mean additional engines to test.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Build.mak
Outdated
| # 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,$(TEST_MXNET_ENGINES))) |
There was a problem hiding this comment.
It'd be nice to be able to simplify these lines.
Build.mak
Outdated
| endef | ||
|
|
||
| # helper function to generate targets for per-engine test runs | ||
| test_with_engines = $(foreach engine,$2,\ |
There was a problem hiding this comment.
Probably here we can drop $2 and just use $(TEST_MXNET_ENGINES) directly.
Build.mak
Outdated
| # 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)) |
There was a problem hiding this comment.
Is there any way to avoid the $(eval $(call ... here? They both seem to be necessary, otherwise I get a missing separator error.
There was a problem hiding this comment.
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).
| @@ -1,5 +1,7 @@ | |||
| INTEGRATIONTEST := integrationtest | |||
|
|
|||
| TEST_MXNET_ENGINES ?= NaiveEngine ThreadedEngine ThreadedEnginePerDevice | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I don't see why there should be a failure. Because with empty TEST_MXNET_ENGINES you define no additional targets.
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
Perhaps define the TEST_MXNET_ENGINES variable to some "always fails" phony target by default, requiring that user override it.
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
Right, I was talking nonsense. Hm...
There was a problem hiding this comment.
There's a way of defining default to be NativeEngine and filtering it out from the TEST_MXNET_ENGINES.
There was a problem hiding this comment.
TBH I think this is overthinking things. If someone sets the field to empty that's their problem ...
Build.mak
Outdated
|
|
||
| # 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)))) |
There was a problem hiding this comment.
Both eval and call seem necessary here too to avoid a missing separator error.
Build.mak
Outdated
| $(1).stamp: $(1)-$(2).stamp | ||
|
|
||
| $(1)-$(2).stamp: $1 | ||
| $(call exec,MXNET_ENGINE_TYPE=$2 $1,$1,$2) |
There was a problem hiding this comment.
Don't you miss a $Vtouch $(1)-$(2).stamp here?
There was a problem hiding this comment.
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).
Build.mak
Outdated
|
|
||
| $O/%unittests: override LDFLAGS += -lz | ||
|
|
||
| # run unittests with all specified engines | ||
| $(eval $(call test_with_engines,$O/allunittests)) |
There was a problem hiding this comment.
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 ... ?
… types This should get the dependencies working OK.
|
I've pushed a further fixup patch. @leandro-lucarella-sociomantic @nemanja-boric-sociomantic you may want to review this solution ;-) |
| $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))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 $@
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
_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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes, I agree, this looks like it needs makd support.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@leandro-lucarella-sociomantic your input is needed here :-)
| download-mnist)) | ||
|
|
||
| # run integration tests with all specified engines | ||
| $(eval $(call run_test_with_engines,$O/test-mxnet)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
OTOH this works just fine, with no re-running of unittests after they have been successfully run once.
|
Considering how many successive issues we're uncovering, I'm starting to question whether this PR is worth the complications it introduces. I wonder whether it might be worth trying to distill the essence of what is wanted here (i.e. ability to replay both unittests and integration tests with multiple different runtime environment settings), and implement support directly in makd, to ensure no conflicts or duplication of effort. |
|
Removing the milestone and lowering the priority, as I think we should take a bit of a step back from this: so far it's proving more complicated than it's worth for the benefit. Would still be interested in addressing the slightly bigger picture feature properly at some point, though. |
I saw @nemanja-boric-sociomantic already reviewed this, I will skip it then . If you still need my input pplease mention me again! |
|
@leandro-lucarella-sociomantic it would be good to have your input on the outcome of our discussions, in particular w.r.t. our discussion point here: #71 (comment) ... but I don't think you need to approach this with any urgency. I can also file an upstream makd issue distilling the requirements (which could be addressed either with a concrete how-to-do-it-downstream solution, or by makd features, or a bit of both, depending on what's required). |
|
To be honest the discussion is too long and it would take a lot of time to go through it, so if it is an option to write a more generic and summarized issue in makd, that would be the best option IMHO (if appropriate, i.e. if there is anything wrong with it or a new feature is needed). |
This patch updates
Build.makto ensure that unittests and integration tests will always be run for all of the supported engine types. As well as making it easier to run tests for all engines locally, this will make it possible to greatly simplify our CI setup.