download-mnist script does not pick up on custom MNIST_DATA_DIR

[joseph-wakeling-sociomantic]

My Config.local.mak defines a custom MNIST_DATA_DIR:

MNIST_DATA_DIR := $(HOME)/data/mnist

However, it seems this is not being picked up by the download-mnist script, which uses the default download directory as set according to:

download_dir=${1-${MNIST_DATA_DIR-"${HOME}/.cache/mnist"}}

The download-mnist target should work by passing the parameter as the first argument to the script:

download-mnist: $C/script/download-mnist
    $(call exec,sh $(if $V,,-x) $^,$(MNIST_DATA_DIR),$^)

... but this appears to not be happening:

$ make V=1 download-mnist 
sh -x script/download-mnist 

[jens-mueller-sociomantic]

I think your configured MNIST_DATA_DIR is not picked by the script because when it executed there is no MNIST_DATA_DIR in the environment. There is no export MNIST_DATA_DIR in your Config.local.mak. Can you test whether this is the case? I think the idea is that

$(call exec,sh $(if $V,,-x) $^,$(MNIST_DATA_DIR),$^)

somehow passes it to the script. But then it needs to be

$(call exec,sh $(if $V,,-x) $^ $(MNIST_DATA_DIR),$(MNIST_DATA_DIR),$^)

it seems. Note, the comma is replaced with a space.

[jens-mueller-sociomantic]

If the latter change is good the script can be simplified.

download_dir=${1-"${HOME}/.cache/mnist"}

But it seems hard to get output of make download-mnist to be good in both cases. Because if MNIST_DATA_DIR is unset it prints the name of the target, i.e., script/download-mnist download-mnist instead of script/download-mnist.

[joseph-wakeling-sociomantic]

The intention to the script was that it should allow the data dir to be passed as the first script argument, or pick it up from the environment. So I don't think we need change the script.

[jens-mueller-sociomantic]

Then export should do the trick. When it does you can close this issue.

[joseph-wakeling-sociomantic]

Yea, I think it's possible I was misremembering the expected usage of the make target; that the first-argument choice is for manual use of the script, whereas make is expected to use the environment-var approach. I'll verify.

[joseph-wakeling-sociomantic]

Incidentally, trying this out shows that there's a bug in Build.mak: the integration-test unittests actually load the dataset, meaning that they also need to depend on the download availability.

[joseph-wakeling-sociomantic]

It also looks like this kind of target/prerequisite setup does not work for the download-mnist script: https://github.com/sociomantic-tsunami/dmxnet/blob/5b70c28f0a0867a06d5e5abc5af7971a4a7c22fe/Build.mak#L23

... in a similar way to problems we've been having defining per-test-target runtime prerequisites in https://github.com/sociomantic-tsunami/dmxnet/pull/71.

[joseph-wakeling-sociomantic]

Example case: if I manually write:

$O/allunittests.stamp: download-mnist

... then we get correct behaviour. But if I use $O/%unittests.stamp instead, the prerequisite is ignored.

My guess would be that this is related to the order of expansion of makefile elements, meaning that when the $O/%unittests.stamp is expanded, there are no matching targets defined yet.

[jens-mueller-sociomantic]

It might be related to step 4 for the implicit rule search. Because when you add commands to the rule it works. But then the already provided commands are overwritten I'll guess.

[jens-mueller-sociomantic]

It seems make is working as designed (see Stackoverflow on Makefile: implicit rule prerequisite not working?).

So maybe just adding the lines

# extra runtime dependencies for unittests                                      
$O/fastunittests.stamp: download-mnist                                          
$O/allunittests.stamp: download-mnist

is fine to make sure that the data set is present when running the unittests. Alternatively, one might exclude the unittests in integrationtests/ when running unittests.

[joseph-wakeling-sociomantic]

Well, I do question why the unittests are attempting to load the data and verify its properties. The verification looks like something the integration tests should do themselves!

I think this is probably the correct solution to the short-term problem -- remove a dependency that shouldn't exist -- but we may still want to consider the more general problem for other use-cases.

[jens-mueller-sociomantic]

I'll guess this can be closed if the problem was a missing export @joseph-wakeling-sociomantic ?

[joseph-wakeling-sociomantic]

Yea, let's close. We can always revisit another time.