Skip to content

Conversation

@yogeshg
Copy link
Contributor

@yogeshg yogeshg commented Mar 3, 2018

The __del__ method that explicitly detaches the object was moved from JavaParams to JavaWrapper class, this way model summaries could also be garbage collected in Java. A test case was added to make sure that relevant error messages are thrown after the objects are deleted.

I ran pyspark tests agains pyspark-ml module
./python/run-tests --python-executables=$(which python) --modules=pyspark-ml

@jkbradley
Copy link
Member

add to whitelist

@SparkQA
Copy link

SparkQA commented Mar 3, 2018

Test build #87920 has finished for PR 20724 at commit d36c1a1.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 3, 2018

Test build #87923 has finished for PR 20724 at commit 07e1829.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.


try:
summary.__del__()
except:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need try...catch here ?
Does it mean when call summary.__del__ after model.__del__ will raise exception ?
Will summary be destroyed at the same time when model.__del__() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__del__ is not a method of the object class. This test throws an error with earlier code (when __del__ is in JavaParams) because the LinearRegressionSummary class did not inherit the del method from any of its ancestors (JavaWrapper and object). After moving the del method to JavaWrapper this line executes. If I remove the try method, then we are testing the condition that "__del__ method exists && __del__ method releases memory".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified offline: This was needed to run the test before the fix in wrapper.py to verify that it failed before the fix.

@jkbradley
Copy link
Member

Thanks @yogeshg and @WeichenXu123 ! I verified the new test fails without the wrapper fix.
LGTM
Merging with master

I won't bother backporting this since it's a small issue.

@asfgit asfgit closed this in 7706eea Mar 5, 2018
@yogeshg yogeshg deleted the java_wrapper_memory branch March 24, 2018 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants