sociomantic-tsunami / dmxnet

D bindings and wrapper library for the MXNet deep learning library
Boost Software License 1.0
14 stars 11 forks source link

Add support for auto-converted D2 releases #70

Closed joseph-wakeling-sociomantic closed 6 years ago

joseph-wakeling-sociomantic commented 6 years ago

This patch updates the Travis CI config to auto-convert tagged releases to D2 and generate a corresponding +d2 release tag. The changes use a similar approach to that used in ocean's CI setup.

Fixes sociomantic-tsunami/dmxnet#67.

jens-mueller-sociomantic commented 6 years ago

I'll guess this cannot be simplified. I would need to read on Travis documentation. It looks kind of complicated. At least more complicated than I expected.

joseph-wakeling-sociomantic commented 6 years ago

@leandro-lucarella-sociomantic how should we test the actual auto-conversion part? I want to make sure we don't accidentally generate a real release ;-)

nemanja-boric-sociomantic commented 6 years ago

What we do in the ocean is that we keep adding patch releases until it works :-). I would love to see if there's a way to test it easily without going through it.

mihails-strasuns-sociomantic commented 6 years ago

I would love to see if there's a way to test it easily without going through it.

Enable travis for your personal fork of the repo and make release there first.

joseph-wakeling-sociomantic commented 6 years ago

Enable travis for your personal fork of the repo and make release there first.

Ah, yes. I should have remembered that trick as we already did it with our upstream mxnet fork. I'll give it a go.

joseph-wakeling-sociomantic commented 6 years ago

BTW, it's not part of this PR, but if anyone has a better idea for how to do all the MXNET_ENGINE_TYPE tests, I'd be all ears ;-) The idea here is that we want to guarantee that for any build settings (compiler, distro, build-type) we will run unittests and integration tests for each of the different MXNet engines.

mihails-strasuns-sociomantic commented 6 years ago

It looks reasonable to me if this is something that has to be chosen at build time

joseph-wakeling-sociomantic commented 6 years ago

OK, looks like that missing DIST is important:

diff --git a/.travis.yml b/.travis.yml
index c4e76f4..24f4d85 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -70,6 +70,6 @@ jobs:

         - stage: D2 Release
           if: tag IS present AND NOT tag =~ \+d2$
-          env: DMD=dmd-transitional F=devel DFLAGS=-vgc
+          env: DMD=dmd-transitional DIST=xenial F=devel
           install: beaver dlang install
           script: beaver dlang d2-release
joseph-wakeling-sociomantic commented 6 years ago

@mihails-strasuns-sociomantic

It looks reasonable to me if this is something that has to be chosen at build time

It's a runtime environment variable, hence why I'd rather not have it in the CI config. The ideal would be that make test would just do a unittest run and integration test run for each of the possible values of this environment variable.

mihails-strasuns-sociomantic commented 6 years ago

Simplest option is change you build.mak like this to override default rule:

# extra runtime dependencies for integration tests
$O/test-mxnet.stamp: override ITFLAGS += $(MNIST_DATA_DIR)
$O/test-mxnet.stamp: download-mnist
    MXNET_ENGINE_TYPE=A $O/test-mxnet
    MXNET_ENGINE_TYPE=B $O/test-mxnet
    MXNET_ENGINE_TYPE=C $O/test-mxnet

This won't run tests in parallel though, that needs defining each as individual make target. But if this is not a concern this should be the simplest option.

joseph-wakeling-sociomantic commented 6 years ago

No, this should be good. Would something similar work for unittests ... ?

mihails-strasuns-sociomantic commented 6 years ago

Yeah, try this:

$O/allunittests.stamp: $O/allunittests
    MXNET_ENGINE_TYPE=A $(call exec,$<,$<,run A)
    MXNET_ENGINE_TYPE=B $(call exec,$<,$<,run B)
    MXNET_ENGINE_TYPE=C $(call exec,$<,$<,run C)
    $Vtouch $@
joseph-wakeling-sociomantic commented 6 years ago

I like it, thanks! Much nicer than the nested-make-call approach we thought of when originally setting this up.

joseph-wakeling-sociomantic commented 6 years ago

This fails because of a missing OAUTH token, but from the build log it looks like we're good to go:

[detached HEAD a7770ed] Auto-convert to D2
 8 files changed, 22 insertions(+), 22 deletions(-)
+d2tag=v0.4.0-alpha.2+d2
++cat
git tag -F- v0.4.0-alpha.2+d2
./submodules/beaver/bin/dlang/d2-release: 38: ./submodules/beaver/bin/dlang/d2-release: GITHUB_OAUTH_TOKEN: parameter not set

Any further feedback on any other issues?

mihails-strasuns-sociomantic commented 6 years ago

Looks good!