You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by wardviaene <gi...@git.apache.org> on 2014/09/05 15:15:00 UTC
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
GitHub user wardviaene opened a pull request:
https://github.com/apache/spark/pull/2287
[SPARK-3415] [PySpark] removes SerializingAdapter code
This code removes the SerializingAdapter code that was copied from PiCloud
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wardviaene/spark feature/pythonsys
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2287.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2287
----
commit e263bf557148ab878e656f3138f6f7cb2cd003fb
Author: Ward Viaene <wa...@bigdatapartnership.com>
Date: 2014-09-05T13:12:03Z
SPARK-3415: removes legacy SerializingAdapter code
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54769479
The Mima failures were an unintended consequence of bumping the version number without changing Mima configurations (see #2315). This looks good to me, so I'm going to merge it into master. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54694441
Jenkins, this is ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17192524
--- Diff: python/pyspark/cloudpickle.py ---
@@ -691,13 +699,13 @@ def save_file(self, obj):
tmpfile.close()
if tst != '':
raise pickle.PicklingError("Cannot pickle file %s as it does not appear to map to a physical, real file" % name)
- elif fsize > SerializingAdapter.max_transmit_data:
- raise pickle.PicklingError("Cannot pickle file %s as it exceeds cloudconf.py's max_transmit_data of %d" %
- (name,SerializingAdapter.max_transmit_data))
+ elif fsize > self._conf.get("max_transmit_data"):
+ raise pickle.PicklingError("Cannot pickle file %s as it exceeds spark.cloudpickle.max_transmit_data of %d" %
+ (name,self._conf.get("max_transmit_data")))
else:
try:
tmpfile = file(name)
- contents = tmpfile.read(SerializingAdapter.max_transmit_data)
+ contents = tmpfile.read(self._conf.get("max_transmit_data"))
--- End diff --
Basically, couldpickle is on third-party module, so it's better to keep it alone (without depending on other parts of spark).
So I think it's fine to remove these about SerializingAdapter, we could have no size limitation for files.
Also, I had tried to do similar things in PR #2144
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54636076
Hi @wardviaene,
Do you have an example program that reproduces this bug? We should probably add it as a regression test (see `python/pyspark/tests.py` for examples of how to do this).
(For other reviewers: you can browse SerializingAdapter's code at http://pydoc.net/Python/cloud/2.7.0/cloud.transport.adapter/) It looks like this code is designed to handle the pickling of file() objects. The Dill developers have recently been discussing how to pickle file handles: https://github.com/uqfoundation/dill/issues/57
It looks like `SerializingAdapter.max_transmit_data` acts as an upper-limit on the sizes of closures that PiCloud would send to their service. Unlike PiCloud, we don't have limits on closure sizes (there are warnings, but these are detected / enforced inside the JVM). Therefore, I wonder if we should just remove this limit and allow the whole file to be read rather than adding an obscure configuration option.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by wardviaene <gi...@git.apache.org>.
Github user wardviaene commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17211687
--- Diff: python/pyspark/tests.py ---
@@ -181,6 +181,17 @@ def tearDown(self):
sys.path = self._old_sys_path
+# Regression test for SPARK-3415
+class CloudPickleTest(unittest.TestCase):
+ def test_pickling_file_handles(self):
+ from pyspark.cloudpickle import dumps
+ out = sys.stderr
+ r = dumps(out)
+ exp = ('\x80\x02c__builtin__\ngetattr\nq\x00cpyspark.cloudpickle\n'
--- End diff --
There is no load in cloudpickle, so I used the default one in python. Test dumps it, loads it, and compares it against original. Also moved the code under SerializationTestCase
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54707812
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19909/consoleFull) for PR 2287 at commit [`aaf10b7`](https://github.com/apache/spark/commit/aaf10b7a933df556c5165e4319df941475132d72).
* This patch **fails** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by wardviaene <gi...@git.apache.org>.
Github user wardviaene commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54661969
Hi @JoshRosen
I added a test script in this pull request. The sys.stderr in a class triggers the bug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54732324
Woah, looks like this is a test failure due to some unrelated MiMa binary compatibility failures in GraphX... strange, since other PRs are building fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17210951
--- Diff: python/pyspark/tests.py ---
@@ -181,6 +181,18 @@ def tearDown(self):
sys.path = self._old_sys_path
+# Regression test for SPARK-3415
+class CloudPickleTestCase(PySparkTestCase):
--- End diff --
One final naming nit: so far, the classes named `*TestCase` are used for factoring out common setup / teardown code (such as `unittest.TestCase`, `PySparkTestCase`, etc.), while the classes with the actual tests have been called `[ComponentName]Tests` or `Test[ComponentName]`. Therefore, I'd prefer to call this `CloudPickleTests`.
Also, since this is just testing CloudPickle without using any PySpark features, it should extend `unittest.TestCase` instead of `PySparkTestCase` so that we don't run a setup / teardown method for a SparkContext that we never use.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54694306
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54707788
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19909/consoleFull) for PR 2287 at commit [`aaf10b7`](https://github.com/apache/spark/commit/aaf10b7a933df556c5165e4319df941475132d72).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54738674
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54732285
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19929/consoleFull) for PR 2287 at commit [`5f0d426`](https://github.com/apache/spark/commit/5f0d4267fd00c5e2fd6205aad0a3088cd5374779).
* This patch **fails** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54730970
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19929/consoleFull) for PR 2287 at commit [`5f0d426`](https://github.com/apache/spark/commit/5f0d4267fd00c5e2fd6205aad0a3088cd5374779).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by wardviaene <gi...@git.apache.org>.
Github user wardviaene commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54707160
Hi @JoshRosen
The bug would only be triggered in a Class, that's why I initially wrote it like that. My last commit removes the references to SerializingAdapter and adds a test script that calls the save function directly. The test fails before the patch, but succeeds after the patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54726495
This looks fine to me, too, although I have two minor naming nits. Sorry to be so nitpicky on the names and test code, but I'd like this code to be really clean so that it serves as an example for future CloudPickle tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54729390
We have SerializationTestCase, so it's better to put it there.
On Sat, Sep 6, 2014 at 12:59 PM, Josh Rosen <no...@github.com>
wrote:
> This looks fine to me, too, although I have two minor naming nits. Sorry
> to be so nitpicky on the names and test code, but I'd like this code to be
> really clean so that it serves as an example for future CloudPickle tests.
>
> --
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2287#issuecomment-54726495>.
>
--
- Davies
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17188600
--- Diff: python/pyspark/tests.py ---
@@ -180,6 +180,22 @@ def tearDown(self):
self.sc.stop()
sys.path = self._old_sys_path
+class CloudPickleTestCase(PySparkTestCase):
+ def SetUp(self):
+ PySparkTestCase.setUp(self)
+ def tearDown(self):
+ PySparkTestCase.tearDown(self)
+ def test_CloudPickle(self):
--- End diff --
I'd probably go with `test_cloudpickle` without the camel-case / capitlaization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54714249
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19911/consoleFull) for PR 2287 at commit [`afc4a9a`](https://github.com/apache/spark/commit/afc4a9a358923bab96dcc145f30b4d6b5b2991be).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17210968
--- Diff: python/pyspark/tests.py ---
@@ -181,6 +181,18 @@ def tearDown(self):
sys.path = self._old_sys_path
+# Regression test for SPARK-3415
+class CloudPickleTestCase(PySparkTestCase):
+ def test_pickling_file_handles(self):
+ from pyspark.cloudpickle import CloudPickler
+ from StringIO import StringIO
--- End diff --
Do we need the StringIO? Can we just use `pyspark.cloudpickle.dumps`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17211496
--- Diff: python/pyspark/tests.py ---
@@ -181,6 +181,17 @@ def tearDown(self):
sys.path = self._old_sys_path
+# Regression test for SPARK-3415
+class CloudPickleTest(unittest.TestCase):
+ def test_pickling_file_handles(self):
+ from pyspark.cloudpickle import dumps
+ out = sys.stderr
+ r = dumps(out)
+ exp = ('\x80\x02c__builtin__\ngetattr\nq\x00cpyspark.cloudpickle\n'
--- End diff --
This isn't guaranteed to be the same across Python versions, so this test will be brittle. It's probably fine to pickle it using dumps, load it with dumps, and verify that what you get back is reasonable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17188918
--- Diff: python/pyspark/tests.py ---
@@ -180,6 +180,22 @@ def tearDown(self):
self.sc.stop()
sys.path = self._old_sys_path
+class CloudPickleTestCase(PySparkTestCase):
+ def SetUp(self):
+ PySparkTestCase.setUp(self)
+ def tearDown(self):
+ PySparkTestCase.tearDown(self)
+ def test_CloudPickle(self):
+ self.t = self.CloudPickleTestClass()
+ a = [ 1 , 2, 3, 4, 5 ]
+ b = self.sc.parallelize(a)
+ c = b.map(self.t.getOk)
+ self.assertEquals('ok', c.first())
+ class CloudPickleTestClass(object):
--- End diff --
Do you need to define a separate class to test this? Maybe a simpler reproduction would be to directly instantiate CloudPickleSerializer and attempt to dump `sys.stderr` directly (or a function that references `sys.stderr` in its closure).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17188526
--- Diff: python/pyspark/tests.py ---
@@ -180,6 +180,22 @@ def tearDown(self):
self.sc.stop()
sys.path = self._old_sys_path
+class CloudPickleTestCase(PySparkTestCase):
+ def SetUp(self):
--- End diff --
This is capitalized (`SetUp`) so it won't be called by `unittest`. Also, we should just end up inheriting the proper setup and teardown methods from PySparkTestCase, so you don't need these methods.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/2287
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54722820
LGTM, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2287#issuecomment-54712596
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19911/consoleFull) for PR 2287 at commit [`afc4a9a`](https://github.com/apache/spark/commit/afc4a9a358923bab96dcc145f30b4d6b5b2991be).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3415] [PySpark] removes SerializingAdap...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2287#discussion_r17188799
--- Diff: python/pyspark/tests.py ---
@@ -180,6 +180,22 @@ def tearDown(self):
self.sc.stop()
sys.path = self._old_sys_path
+class CloudPickleTestCase(PySparkTestCase):
+ def SetUp(self):
+ PySparkTestCase.setUp(self)
+ def tearDown(self):
+ PySparkTestCase.tearDown(self)
+ def test_CloudPickle(self):
--- End diff --
Also, `test_cloudpickle` isn't a very descriptive name; it will be hard for people that come along and read this later to figure out what this is supposed to be testing. A better name would be `test_pickling_file_handles` (and maybe add a comment saying that it's a regression test for SPARK-3415).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org