You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by superbobry <gi...@git.apache.org> on 2018/04/25 15:25:14 UTC

[GitHub] spark pull request #21157: [SPARK-22674][PYTHON] Removed the namedtuple pick...

GitHub user superbobry opened a pull request:

    https://github.com/apache/spark/pull/21157

    [SPARK-22674][PYTHON] Removed the namedtuple pickling patch

    ## What changes were proposed in this pull request?
    
    This is a breaking change.
    
    Prior to this commit PySpark patched ``collections.namedtuple`` to make
    namedtuple instances serializable even if the namedtuple class has been
    defined outside of ``globals()``, e.g.
    
        def do_something():
            Foo = namedtuple("Foo", ["foo"])
            sc.parallelize(range(1)).map(lambda _: Foo(42))
    
    The patch changed the pickled representation of the namedtuple instance
    to include the structure of namedtuple class, and recreate the class on
    each unpickling. This behaviour causes hard to diagnose failures both
    in the user code with namedtuples, as well as third-party libraries
    relying on them. See [1] and [2] for details.
    
    [1]: https://superbobry.github.io/pyspark-silently-breaks-your-namedtuples.html
    [2]: https://superbobry.github.io/tensorflowonspark-or-the-namedtuple-patch-strikes-again.html
    
    ## How was this patch tested?
    
    PySpark test suite.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/criteo-forks/spark no-hijack-namedtuple

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/21157.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 #21157
    
----

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by holdenk <gi...@git.apache.org>.

Github user holdenk commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Is it possible to keep the current hack for things which can't be pickled, but remove the hack in the situation where the namedtuple is well behaved and it could be pickled directly by cloudpickle? That way we don't have a functionality regression but we also improve handling of named tuples more generally. Even if so, it would probably be best to wait for 3.0 since this is a pretty core change in terms of PySpark.
    
    Before you put in the work though let's see if that the consensus approach (if possible).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    ok to test


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by superbobry <gi...@git.apache.org>.

Github user superbobry commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    > Does the test even pass?
    
    The tests should pass module the tests specifically checking the behaviour being removed. I think the failing RDD test is in this group as well. 
    
    > Why don't we try to fix it rather than removing out? 
    
    I might be overly pessimistic but I don't see how we can make the patch work in all cases without making the implementation more magical, and as a result, producing even more confusing error messages when things go wrong. Consider, for instance, a widespread pattern
    
    ```python
    class Foo(namedtuple("Foo", [])):
        def foo(self):
            return 42
    ```
    
    If the outer `Foo` class does not explicitly customize pickling, it would use the "fallback" implementation added by `_hijack_namedtuple`, which only knows about the inner namedtuple class. Therefore, confusingly enough `issubclass(pickle.loads(foo), Foo)` is False (as detailed in [2]).
    
    What can we do about this? We somehow need to serialize the **full definition** of the outer `Foo` class alongside every instance. Maybe this can be done by recursively pickling the class `__name__`, `__bases__` and `__dict__`, but `__dict__` could have some other hard-to-pickle objects like user-defined methods. Should we serialize these in the deconstructed form as well? These are tough questions, and I think they are better left outside the scope of PySpark. 
    
    That said, I think an alternative to completely removing the patch might be deprecating it, and advertizing `cloudpickle` for workloads using namedtuples (or even making it the default?). I've played with `cloudpickle` a little bit, and it seems to solve the aforementioned issues in a consistent manner. The price, however, is the added overhead:
    
    ```python
    >>> len(pickle.dumps(Foo()))
    23
    >>> len(cloudpickle.dumps(Foo()))
    3538
    ```
    
    or, even more extreme,
    
    ```python
    >>> class A: pass
    ...
    >>> len(cloudpickle.dumps(A()))
    177
    ```
    
    What do you think?
    
    [2]: https://superbobry.github.io/tensorflowonspark-or-the-namedtuple-patch-strikes-again.html


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    **[Test build #89878 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89878/testReport)** for PR 21157 at commit [`67c4f67`](https://github.com/apache/spark/commit/67c4f6707aab670b9cde5a3afa34fda3abbbf46d).
     * This patch **fails PySpark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    You can just replace it to CloudPickler, remove changes at tests, and push that commit here to show no case is broken


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by superbobry <gi...@git.apache.org>.

Github user superbobry commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    @HyukjinKwon do you mean change the default serializer to cloudpickle and remove _hack_namedtuple?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Please go ahead if there's another approach to avoid to remove but fix it.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by superbobry <gi...@git.apache.org>.

Github user superbobry commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Yes, we can backport some of the cloudpickle code to make the patch less fragile. This would be a nontrivial change in an already complex code, but I'd be happy to sketch this if there's a consensus on the ML
    
    Also, note that even without the patch it is possible to have an RDD of namedtuples as long as the namedtuple classes are defined inside an importable module, i.e. NOT inside a function/REPL.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Yea, so to avoid to break, we could change the default pickler to CloudPickler or document this workaround. @superbobry, can you check if the case can be preserved if we use CloudPickler instead?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    **[Test build #96815 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96815/testReport)** for PR 21157 at commit [`2addefb`](https://github.com/apache/spark/commit/2addefb9ae2ec12f30e0e939f7782b521efccdd0).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96760/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by superbobry <gi...@git.apache.org>.

Github user superbobry commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Yes, it will break IPython notebooks as well. I wonder how often people actually defined namedtuples in a notebook? 
    
    Emitting a warning is a less extreme option, yes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by superbobry <gi...@git.apache.org>.

Github user superbobry commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    > so this change would introduce a pretty big regression?
    
    The change does introduce a regression as some namedtuples will become unpicklable. However, it makes pickling in PySpark more predictable and robust (see the linked blog posts for details).
    
    Sidenote: `mllib.fpm` contains another case of non-picklable namedtuple -- "nested":
    
    ```python
    >>> class A:
    ...     class B(namedtuple("B", [])): pass
    ...
    >>> import pickle
    >>> pickle.loads(pickle.dumps(A.B))
    Traceback (most recent call last):
      [...]
    pickle.PicklingError: Can't pickle <class '__main__.B'>: it's not found as __main__.B
    ```
    
    It is pickleable with `_hijack_namedtuple` enabled, because the namedtuple class is recreated during unpickling. However, I think that "nested" classes are an anti-pattern in Python, because the outer class is not a proper namespace (like locals/globals/...):
    
    ```python
    >>> A.B
    <class '__main__.B'>
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89865/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96758/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    **[Test build #89865 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89865/testReport)** for PR 21157 at commit [`eadc0c8`](https://github.com/apache/spark/commit/eadc0c8af853a57ee80f5e80fb708451931eedc0).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    > Ok it looks like it was @HyukjinKwon who suggested that we remove this hack in general rather than the partial work around can I get your thoughts on why? It seems like the partial work around would give us the best of both worlds (e.g. we don't break peoples existing Spark code and we handle Python tuples better).
    
    Sorry for the late response. Yes, I spent some time to take a look for this named tuple hack, and my impression was that we should have not added such fixes to only allow named tuple pickling. The named tuple hack was introduced for both cloudpickle (SQL path) and normal pickle path, if I am not mistaken.
    
    Cloudpickle at PySpark side now supports it so workaround to allow the cases above should be to use CloudPickler when it's possible. I think PySpark API exposes this pickler (see the `SparkContext`'s `__init__`) (@superbobry, mind if I ask to document this workaround and add a test (and see if it really works?)



---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by superbobry <gi...@git.apache.org>.

Github user superbobry commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Closing this PR to continue the discussion in the new one.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by rxin <gi...@git.apache.org>.

Github user rxin commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    so this change would introduce a pretty big regression?



---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Adding @gatorsmile and @cloud-fan as well since this might be potentially breaking changes for 3.0 release (it affects RDD operation only with namedtuple in certain case tho)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by holdenk <gi...@git.apache.org>.

Github user holdenk commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    If removing the hack entirely is going to brake named tuples defined in the repl I'm a -1 on that change. While we certainly are more free to make breaking API changes in a majour version release we still have to think through the scope of the change we're going to be pushing onto users and that's pretty large.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #21157: [SPARK-22674][PYTHON] Removed the namedtuple pickling pa...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/21157
  
    **[Test build #96813 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96813/testReport)** for PR 21157 at commit [`9c69047`](https://github.com/apache/spark/commit/9c690474986be63e41eed7aba59399c5a154b72e).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org