remove mod from arity 2 version of load-checkpoint in clojure-package

[jimdunn]

Description

The arity 1 version of load-checkpoint in the module namespace works, but the arity 2 version just calls itself. Probably not noticed because mod is a clojure function, though it's used throughout the code as a variable name for modules.
Just needed to delete that mod, and arity 2 version properly calls the arity 1 version.

[gigasquid]

@jimdunn Thanks so much for finding and fixing this 😸

We've been trying to improve the coverage of our unit tests, would you mind adding a test for this arity to make sure it doesn't break again? https://github.com/apache/incubator-mxnet/blob/master/contrib/clojure-package/test/org/apache/clojure_mxnet/module_test.clj#L105

[jimdunn]

Oh sure no problem. I'll take care of that in the next day or two.

[jimdunn]

@gigasquid Hey, I have a question about those tests. The arity 2 function has load-optimizer-states as false, which is the default. So I initially thought it might not pass the test because it wouldn't load the optimizer states, but apparently the optimizer state isn't actually checked since my test worked ok.

So I wondered if there was a point of the init-optimizer bit in those tests and found that removing those lines didn't have an effect.
https://github.com/apache/incubator-mxnet/blob/master/contrib/clojure-package/test/org/apache/clojure_mxnet/module_test.clj#L108
I saw that the scala tests for this are basically identical. I tried to run them to see if to see if they would be fine without the initOptimizer step, but I was unable to figure out the scala pacakge install and I don't have time to spend on that.

In any case, I'm not sure what the best thing to do is. I can just add my test "as is" and not worry about this apparently unnecessary step, or even remove that step from the test. But if it's supposed to matter and we care about the optimizer state for these tests, then that should be fixed. The problem for me with this is that I can't see how to access the optimizer state in mod to make the comparison.

Maybe I'm making too much of it, but I found it confusing that this load-optimizer-states is explicitly set in the test and neither it nor the init-optimizer step affect the success of the test.

Am I making sense? Thoughts on this?

[gigasquid]

Makes total sense. Looking at the code for this, it is not very easy to test that the optimizer states are loaded. Basically, the optimizer states are serialized to atest-0000.states file and then loaded into the model. It sets a private variable called preloadOptStates which is then used in the init-optimizer https://github.com/apache/incubator-mxnet/blob/master/scala-package/core/src/main/scala/org/apache/mxnet/module/Module.scala#L404 to set it.

Unfortunately, I can't find a good way to verify this in the tests so I would just recommend to leave them as is unless someone else has a better idea.

Thanks for digging into this and enhancing the tests to make them better 👍 .

[jimdunn]

No problemo. I'm enjoying playing around with this package.
Yeah I figured just leaving them would probably be best for now. I just wanted to bring it up in case I missed something obvious. Little things like that really bug me, but I am sadly not equipped to dig any deeper into this matter right now.

[gigasquid]

This looks good from the Clojure point of view. It's ready for @nswamy or @szha to review.