Skip to content

[MINOR][BUILD][PYSPARK] Disable test_memory_limit test temporarily #27158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HeartSaVioR wants to merge 2 commits into from
Closed

[MINOR][BUILD][PYSPARK] Disable test_memory_limit test temporarily #27158

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07608af
[MINOR][BUILD][PYSPARK] Disable test_memory_limit test temporarily
HeartSaVioR Jan 10, 2020
23ad40b
Revert pip8 violation (my bad)
HeartSaVioR Jan 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions python/pyspark/tests/test_worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,12 @@ def test_reuse_worker_of_parallelize_xrange(self):
for pid in current_pids:
self.assertTrue(pid in previous_pids)


@HyukjinKwon HyukjinKwon Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wouldn't change such newlines BTW. This is per pep8. another newline below, I don't think that violates pep 8.

@HeartSaVioR HeartSaVioR Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad. linter doesn't work in my machine (maybe it's because the the default python in my dev. is 2.7?) so missed that. Will revert.

@unittest.skipIf(
not has_resource_module,
"Memory limit feature in Python worker is dependent on "
"Python's 'resource' module; however, not found.")
class WorkerMemoryTest(PySparkTestCase):

@unittest.skip("disabled temporarily since it's failing consistently")
def test_memory_limit(self):
self.sc._conf.set("spark.executor.pyspark.memory", "1m")

@srowen srowen Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The only other thing I can think of is that this toy memory limit is too low. What if it were 8m or something? if you feel like it you could try testing that too

@HeartSaVioR HeartSaVioR Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The odd thing is that it passes with local dev. (MacOS, python3 - 3.7.4/python2 - 2.7.10/pypy - Python 2.7.13 & pypy 7.3.0 GCC 4.2.1) so not clear it would help, but if we suspect it we can try.

@HyukjinKwon HyukjinKwon Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It will pass on Mac OS because Mac doesn't actually limits the memory. it doesn't work on Windows either. So, this hypothesis could make sense. Can we increase to something like 8m?

@HeartSaVioR HeartSaVioR Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK let me do it instead. Let's modify the PR title/description after confirming it works.

@HyukjinKwon HyukjinKwon Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe it's best to file a JIRA btw.

@HeartSaVioR HeartSaVioR Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about filing an issue after confirming the trick doesn't work and we have to disable? We may be able to just treat this PR as MINOR but mentioning the new JIRA issue in "reason" which is expected to address this.

@HyukjinKwon HyukjinKwon Jan 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure, that works too. FWIW, I doubt if it consistently fails in the master yet. If that's the case, it should better fix the test here.

@HeartSaVioR HeartSaVioR Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI #27159 is the patch increasing the value to 8m

rdd = self.sc.parallelize(xrange(1), 1)
Expand Down