Python memory fix #1214

rcurtin · 2018-01-24T23:42:28Z

This is a set of bugfixes for the Python bindings that improve several issues including fixing #1201. @nvasil: you can apply the patches from here or use this branch and it will fix your memory issue---I tested with the script from #1201.

A more clear summary of changes:

Avoid copies of matrices and other large types by using std::move() when calling CLI::SetParam<>() from the generated Cython bindings.
Clarify and correct ownership of matrix memory: now, in all cases, matrices passed down from Python will retain their ownership of the memory, and internally in mlpack we will use non-strict aliases so that we never delete the user's memory.
As a result of the code above, any binding that uses std::move() on the input matrix (most of them do) will now result in the Python user's matrix not being empty.
Also we print default values for optional string/double/int parameters now, which is nice.

There is more to do before I merge this, though:

Handle the model ownership issue: right now an input model gets wiped out and the user has to use copy.copy(). Instead we should have CLI hold pointers to models instead, which allows us to make output_model point to the same model as input_model.
Force copies by default but provide a clear warning that extra copies are being done. We can add an option make_copies and if make_copies=False, then we use aliases everywhere but the user's input matrices or models may be modified. I'm not sure if I like defaulting make_copies to True or False; there are advantages and disadvantages to each. I'm slightly inclined to set make_copies=False by default, then make clear in the documentation that if "weird things" are happening, try make_copies=True.

In any case, as it stands this fixes #1201.

This fixes mlpack#1201 but does collateral damage: now every matrix that is passed in will be modified. This will be fixed next...

First: don't transfer the memory state to Armadillo. Instead, make a non-strict alias; this means that Armadillo will only allocate new memory on a size change, and we won't end up making the Python user's numpy array be nothing when we're done because we std::move()d the memory. Second: Only transfer the ownership of the matrix to numpy if the Armadillo matrix actually allocated its own memory. This can help with situations where the user passes a numpy matrix that they have a reference to that will also be a part of the output.

ShikharJ · 2018-01-25T14:36:52Z

@rcurtin Why is the #1200 referenced in the last line? I'm assuming you meant to refer to #1201?

rcurtin · 2018-01-25T14:47:02Z

Because I remembered the correct number wrong when I originally wrote the description, then I went back and changed it but missed the third reference. :)

Specifically, whereas before any serializable type would actually be held as a std::tuple<ModelType, std::string>, but now we will hold this as a std::tuple<ModelType*, std::string>. This also requires some changes for memory handling. Our assumption is that the CLI object will *own* (and delete) any pointers it is holding, so we must also do some extra handling in the end_program.hpp code.

If we make a copy during the conversion, then Armadillo should own the memory (otherwise Python will delete the temporary).

This refactors the Python bindings to deal with holding pointers to serializable types. Some minor extra work is necessary to prevent Python from accidentally deleting models multiple times.

This is hand-tested as working, but I need to write actual tests still.

(This is probably not completely correct but I am moving systems right now.)

rcurtin · 2018-01-29T18:08:57Z

Once I have this passing all the tests (we will see if it does) I think this is ready. The key change to the Python bindings is that now I can do the following:

>>> from mlpack import knn
>>> import numpy as np
>>> x = np.random.rand(10, 10)
>>> out = knn(reference=x, k=3, verbose=True)

That will (or may) modify x, but we can force a copy:

>>> out = knn(reference=x, k=3, verbose=True, copy_all_inputs=True)

and now x will be copied before the binding is run. This option is documented and should obviate the need for the clunky copy.copy() I was suggesting before. By default inputs are not copied for the sake of speed (including models).

rcurtin · 2018-01-29T18:37:15Z

@mlpack-jenkins build this

zoq · 2018-01-29T19:19:48Z

@mlpack-jenkins test this please

zoq · 2018-01-30T21:43:27Z

src/mlpack/bindings/cli/get_allocated_memory.hpp

+                        const void* /* input */,
+                        void* output)
+{
+  *((void**) output) =


Such a pretty line 😃

I wish I knew a better way to do it, but the only way I could seem to was by casting around void pointers in all kinds of strange ways...

zoq · 2018-01-30T21:50:24Z

src/mlpack/bindings/python/print_input_processing.hpp

@@ -31,6 +31,11 @@ void PrintInputProcessing(
    const typename boost::disable_if<std::is_same<T,
        std::tuple<data::DatasetInfo, arma::mat>>>::type* = 0)
 {
+  // The copy_all_inputs parameter must be handled first, and so is outside the


and so is outside the

Is this correct?

Yeah, I guess it could also be written and therefore is outside the; I went ahead and changed it for clarity.

zoq · 2018-01-30T21:51:51Z

src/mlpack/bindings/python/print_input_processing.hpp

-   *       MoveFromPtr[Model](CLI.GetParam[Model]('param_name'),
-   *           (<ModelType> param_name).modelptr)
+   *       SetParamPtr[Model]('param_name', (<ModelType> param_name).modelptr,
+   *           CLI.HasParam('copy_all_inputs'))  TODO


Is this done?

Yeah, I wrote that down when I was getting up to get dinner, and when I came back and wrote the code apparently I forgot to remove the TODO... :)

zoq · 2018-01-30T21:56:13Z

src/mlpack/bindings/tests/delete_allocated_memory.hpp

@@ -0,0 +1,56 @@
+/**
+ * @file delete_allocated_memory.hpp


Is there any difference between this file and the file in src/mlpack/bindings/cli/delete_allocated_memory.hpp? Maybe I missed something.

In this case yes, the CLI bindings have to delete the first element of a tuple<T*, std::string>, but the test bindings only hold the T* directly so the delete command is a little different.

I see, thanks for the clarification.

zoq

Looks good to me. No more comments.

rcurtin · 2018-02-05T15:31:45Z

Ok, I will go ahead and merge this then.

…rse_coding_main.cpp.

…in sparse_coding_main.cpp.

rcurtin added 4 commits January 24, 2018 11:29

Take ownership of given parameters to fix memory leak.

7f0c453

This fixes mlpack#1201 but does collateral damage: now every matrix that is passed in will be modified. This will be fixed next...

Print default values in the help.

7f3f451

This import is no longer needed.

86ffadc

rcurtin added 21 commits January 26, 2018 13:44

Fix memory handling of numpy arrays.

84427e1

If we make a copy during the conversion, then Armadillo should own the memory (otherwise Python will delete the temporary).

Make Python bindings hold pointers.

7903a0e

This refactors the Python bindings to deal with holding pointers to serializable types. Some minor extra work is necessary to prevent Python from accidentally deleting models multiple times.

Update test bindings to hold model pointers.

7e2dcf9

Update PARAM_MODEL_IN() and PARAM_MODEL_OUT() macros to hold pointers.

7c53e3e

Refactor all programs to work with model pointers.

ede3686

Oops, this snuck in somehow.

6b954eb

Remove move.hpp from build configuration.

6e3f0cb

Adapt tests to use pointers to models.

b773276

Remove code that wasn't needed in the end.

934682e

Fix too-long lines.

cb829ac

Add a 'copy_all_inputs' option to Python bindings.

787e090

This is hand-tested as working, but I need to write actual tests still.

Add tests for Python bindings and update for new to_matrix() API.

6f88288

Merge remote-tracking branch 'upstream/master' into python-memfix

580adc9

Update tests to use pointers.

3b756a0

(This is probably not completely correct but I am moving systems right now.)

Add functions to clean up after tests.

c7dc28b

Fix subtle memory leak.

fdb3799

Add tests for GetAllocatedMemory() and DeleteAllocatedMemory().

2a5304b

Add header guards.

1096b26

Correctly handle memory in all main tests.

9d59cc8

Fix comments.

a402f61

zoq reviewed Jan 30, 2018

View reviewed changes

Minor comment fixes.

ac89b49

zoq approved these changes Feb 1, 2018

View reviewed changes

rcurtin merged commit fcd5ccf into mlpack:master Feb 5, 2018

This was referenced Feb 5, 2018

Binding test for hoeffding_tree #1211

Merged

Binding Tests #1199

Merged

AdaBoost binding tests #1220

Merged

rcurtin deleted the python-memfix branch February 5, 2018 15:55

nikhilgoel1997 added a commit to nikhilgoel1997/mlpack that referenced this pull request Feb 5, 2018

Update to handle changes in mlpack#1214

94c07f9

rcurtin added a commit that referenced this pull request Feb 5, 2018

Update test to handle memory as per #1214, and fix a few leaks in spa…

c107ffc

…rse_coding_main.cpp.

rcurtin mentioned this pull request Feb 5, 2018

Sparse Coding Bindings Tests #1217

Merged

rcurtin pushed a commit to rcurtin/mlpack that referenced this pull request Feb 15, 2018

Update to handle changes in mlpack#1214

244b0f3

rcurtin added a commit to rcurtin/mlpack that referenced this pull request Feb 15, 2018

Update test to handle memory as per mlpack#1214, and fix a few leaks …

0e02c33

…in sparse_coding_main.cpp.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python memory fix #1214

Python memory fix #1214

rcurtin commented Jan 24, 2018 •

edited

ShikharJ commented Jan 25, 2018

rcurtin commented Jan 25, 2018

rcurtin commented Jan 29, 2018

rcurtin commented Jan 29, 2018

zoq commented Jan 29, 2018

zoq Jan 30, 2018

rcurtin Jan 31, 2018

zoq Jan 30, 2018

rcurtin Jan 31, 2018

zoq Jan 30, 2018

rcurtin Jan 31, 2018

zoq Jan 30, 2018

rcurtin Jan 31, 2018

zoq Feb 1, 2018

zoq left a comment

rcurtin commented Feb 5, 2018

Python memory fix #1214

Python memory fix #1214

Conversation

rcurtin commented Jan 24, 2018 • edited

ShikharJ commented Jan 25, 2018

rcurtin commented Jan 25, 2018

rcurtin commented Jan 29, 2018

rcurtin commented Jan 29, 2018

zoq commented Jan 29, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoq left a comment

Choose a reason for hiding this comment

rcurtin commented Feb 5, 2018

rcurtin commented Jan 24, 2018 •

edited