[Clojure] Add resource scope to clojure package #13993

gigasquid · 2019-01-25T16:25:10Z

Description

Ports ResourceScope from the Scala package and introduces using macro to automatically close any resource in the enclosing forms.

#13442

and see https://cwiki.apache.org/confluence/display/MXNET/JVM+Memory+Management for more context

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Also added the resource scope to the imclassification example.

New runs to the example do not show any warnings of memory leaks of this nature:
WARN org.apache.mxnet.WarnIfNotDisposed: LEAK: [one-time warning] ...

benkamphaus

The implementation looks good to me. The tests are technically sound and illustrate the desired behavior. My only comment is just a nit and question: is there a use case (perhaps one someone encountered) that would make sense as a more natural example for the tests but could still be easily tested? (.e.g with the isDisposed method or otherwise).

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj

kedarbellare · 2019-01-26T05:06:04Z

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj

+    (is (true? (.isDisposed (:temp-x @native-resources))))
+    (is (false? (.isDisposed x)))))
+
+(deftest test-resource-scope-with-sym


can you also add a unit test for nested with sym?

one more unit test idea (similar in spirit to scala api): many ndarray/ones within a vector and only the first one is returned after a transformation (e.g. (+ (first v) 1))

interesting the result here - I put test cases in, but if a whole vector has been assigned to a let binding and you return the first, the whole vector is not disposed. You have to call copy on the first or not assign the whole vector to a let to have it disposed

There are some edge cases still I think there - but on the whole, it's an improvement for mem management of native resources.

kedarbellare · 2019-01-26T05:13:26Z

contrib/clojure-package/examples/imclassification/src/imclassification/train_mnist.clj


 (defn -main [& args]
  (let [[dev dev-num] args
        devs (if (= dev ":gpu")
               (mapv #(context/gpu %) (range (Integer/parseInt (or dev-num "1"))))
               (mapv #(context/cpu %) (range (Integer/parseInt (or dev-num "1")))))]
-    (start devs)))
+    (resource-scope/using (start devs))))


having the whole thing encapsulated in one resource-scope seems weird to me. is it possible to wrap using around the individual pieces (e.g. mnist-iter, fit, fit-params) so that most of the code remains the same?

that's a good idea - thanks

contrib/clojure-package/src/org/apache/clojure_mxnet/resource_scope.clj

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj

gigasquid · 2019-01-26T15:27:06Z

Thanks for your review @kedarbellare - I'll work on implementing the feedback 😸

kedarbellare · 2019-01-26T15:43:51Z

contrib/clojure-package/src/org/apache/clojure_mxnet/resource_scope.clj

+(defmacro using
+  "Uses a Resource Scope for all forms. This is a way to manage all Native Resources like NDArray and Symbol - it will deallocate all Native Resources by calling close on them automatically. It will not call close on Native Resources returned from the form"
+  [& forms]
+  `(ResourceScope/using (new ResourceScope) (util/forms->scala-fn ~@forms)))


this may be a stupid question but is there any value in reusing a previously defined ResourceScope object? why does the scala API allow a scope to be passed in?

That's a good question. From looking at the Scala api I would say that it would enable finer grained control - you could manually add or remove native resources from a scope and then manually call close on it. But I don't think that we need to expose that on the Clojure side, unless you have a different view on it.

i don't see a big need for it -- at least for now -- but i was wondering whether the motivation was something like tf.variable_scope (e.g. reusing symbols).

I don't know - maybe @nswamy knows more about the context behind the design? ^

gigasquid · 2019-01-28T17:03:31Z

I had some stuff come up - but plan to finish this up sometime this week :)

- move from defs to atoms to make the tests a bit better

more tests

gigasquid · 2019-02-01T20:33:54Z

I think all the feedback is addressed. Feel free to take another look. If there are no more changes needed, I will merge 😸

kedarbellare · 2019-02-02T15:27:16Z

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj

+                                              (swap! native-resources conj x)
+                                              x)))))]
+    (is (false? (ndarray/is-disposed return-val)))
+    (is (every? true? (mapv ndarray/is-disposed (:temp-ndarrays @native-resources))))))


@native-resources is a vector. just check (mapv ndarray/is-disposed @native-resources)? (also i didn't know that (:keyword vector) didn't raise an error)

If fixed some other problems with laziness in the tests with repeatedly. Evidently the weird behavior in the dispose of the first of the collection was just my fault on the tests. Now everything works as expected 🎆

kedarbellare · 2019-02-02T15:29:30Z

contrib/clojure-package/examples/imclassification/src/imclassification/train_mnist.clj

-                    :eval-data test-data
+      (resource-scope/with-let [_mod (m/module (get-symbol) {:contexts devs})]
+        (-> _mod
+            (m/fit {:train-data (mx-io/mnist-iter {:image (str data-dir "train-images-idx3-ubyte")


just for my understanding, does resource-scope not work when the train-data and eval-data are defined outside the with-let block?

It will work if they are defs outside, but you will still get the undisposed ndarray warning. I refactored to move it out to functions which does work and looks better too :)

now they work as expected!

kedarbellare · 2019-02-02T16:21:03Z

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj

+    (is (false? (sym/is-disposed x)))))
+
+;;; Note that if first is returned the rest of the collection ndarrays will
+;;; NOT be disposed


update comment?

oh yes - the curse of the leftover misleading comments :)

kedarbellare

neat!! looks great! 🎉

kedarbellare · 2019-02-02T16:22:10Z

can't wait to use this 😃

@benkamphaus

* Add resource scope to clojure package * add rat * fix integration test * feedback from @benkamphaus - move from defs to atoms to make the tests a bit better * adding alias with-do and with-let more tests * another test * Add examples in docstring * refactor example and test to use resource-scope/with-let * fix tests and problem with laziness now they work as expected! * refactor to be a bit more modular * remove comments

@benkamphaus

* Add resource scope to clojure package * add rat * fix integration test * feedback from @benkamphaus - move from defs to atoms to make the tests a bit better * adding alias with-do and with-let more tests * another test * Add examples in docstring * refactor example and test to use resource-scope/with-let * fix tests and problem with laziness now they work as expected! * refactor to be a bit more modular * remove comments

@benkamphaus

* Add resource scope to clojure package * add rat * fix integration test * feedback from @benkamphaus - move from defs to atoms to make the tests a bit better * adding alias with-do and with-let more tests * another test * Add examples in docstring * refactor example and test to use resource-scope/with-let * fix tests and problem with laziness now they work as expected! * refactor to be a bit more modular * remove comments

gigasquid added the Clojure label Jan 25, 2019

benkamphaus approved these changes Jan 25, 2019

View reviewed changes

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj Outdated Show resolved Hide resolved

kedarbellare reviewed Jan 26, 2019

View reviewed changes

contrib/clojure-package/src/org/apache/clojure_mxnet/resource_scope.clj Outdated Show resolved Hide resolved

kedarbellare reviewed Jan 26, 2019

View reviewed changes

contrib/clojure-package/test/org/apache/clojure_mxnet/resource_scope_test.clj Show resolved Hide resolved

kedarbellare reviewed Jan 26, 2019

View reviewed changes

gigasquid added 6 commits February 1, 2019 10:49

Add resource scope to clojure package

444f6f4

add rat

a44e5a6

fix integration test

329611d

feedback from @benkamphaus

f4a38d9

- move from defs to atoms to make the tests a bit better

adding alias with-do and with-let

32c9bfe

more tests

another test

a5db1f2

gigasquid force-pushed the clojure-resource-scope branch from 186a8aa to a5db1f2 Compare February 1, 2019 17:01

gigasquid added 2 commits February 1, 2019 15:16

Add examples in docstring

5ae5085

refactor example and test to use resource-scope/with-let

e26836e

kedarbellare approved these changes Feb 2, 2019

View reviewed changes

kedarbellare reviewed Feb 2, 2019

View reviewed changes

gigasquid added 2 commits February 2, 2019 10:56

fix tests and problem with laziness

fdb1e73

now they work as expected!

refactor to be a bit more modular

d8ab1b7

kedarbellare reviewed Feb 2, 2019

View reviewed changes

kedarbellare approved these changes Feb 2, 2019

View reviewed changes

gigasquid added 2 commits February 2, 2019 11:23

remove comments

7aa65f9

Merge branch 'master' into clojure-resource-scope

9e5cfc4

gigasquid merged commit fbc5723 into apache:master Feb 5, 2019

gigasquid deleted the clojure-resource-scope branch February 5, 2019 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clojure] Add resource scope to clojure package #13993

[Clojure] Add resource scope to clojure package #13993

gigasquid commented Jan 25, 2019

benkamphaus left a comment

kedarbellare Jan 26, 2019

kedarbellare Jan 26, 2019 •

edited

Loading

gigasquid Feb 1, 2019

gigasquid Feb 1, 2019

kedarbellare Jan 26, 2019 •

edited

Loading

gigasquid Jan 26, 2019

gigasquid commented Jan 26, 2019

kedarbellare Jan 26, 2019

gigasquid Jan 26, 2019

kedarbellare Jan 26, 2019 •

edited

Loading

gigasquid Jan 26, 2019

gigasquid commented Jan 28, 2019

gigasquid commented Feb 1, 2019

kedarbellare Feb 2, 2019

gigasquid Feb 2, 2019

kedarbellare Feb 2, 2019

gigasquid Feb 2, 2019

kedarbellare Feb 2, 2019

gigasquid Feb 2, 2019

kedarbellare left a comment

kedarbellare commented Feb 2, 2019

[Clojure] Add resource scope to clojure package #13993

[Clojure] Add resource scope to clojure package #13993

Conversation

gigasquid commented Jan 25, 2019

Description

Checklist

Essentials

Changes

benkamphaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarbellare Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarbellare Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gigasquid commented Jan 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarbellare Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gigasquid commented Jan 28, 2019

gigasquid commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarbellare left a comment

Choose a reason for hiding this comment

kedarbellare commented Feb 2, 2019

kedarbellare Jan 26, 2019 •

edited

Loading

kedarbellare Jan 26, 2019 •

edited

Loading

kedarbellare Jan 26, 2019 •

edited

Loading