[BEAM-8123] Add cloudpickle as optional library by ryanthompson591 · Pull Request #15472 · apache/beam

ryanthompson591 · 2021-09-07T19:14:31Z

Wraps the pickle library so that users can choose either cloudpickle or dill.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

ryanthompson591 · 2021-10-29T15:32:28Z

R: @tvalentyn

ryanthompson591 · 2021-10-29T17:24:35Z

Python Unit Tests

ryanthompson591 · 2021-10-29T17:33:30Z

Python Unit Tests

ryanthompson591 · 2021-10-29T18:56:19Z

Python Unit Tests

ryanthompson591 · 2021-10-29T19:31:38Z

Python Unit Tests

sdks/python/apache_beam/coders/coders_test_common.py

sdks/python/apache_beam/internal/cloudpickle_pickler.py

tvalentyn · 2021-11-02T23:53:28Z

sdks/python/apache_beam/internal/cloudpickle_pickler_test.py

+        loads(dumps(module_test.TopClass.MiddleClass.NestedClass('abc'))).datum)
+
+  def test_dynamic_class(self):
+    """Tests that a nested class object is pickled correctly."""


nit: duplicated comment?

You could also name the test so that it captures the behavior; then the comment becomes unnecessary. E.g. smth like:

test_pickling_preserves_closure_of_a_function() test_dynamic_class_objects_pickle_correctly()

sdks/python/apache_beam/internal/cloudpickle_pickler_test.py

tvalentyn · 2021-11-03T00:11:54Z

sdks/python/apache_beam/internal/pickler.py

+
+def change_pickle_lib(pickle_lib):
+  """ Changes pickling library. Users should prefer the default library."""
+  global pickler_lib


how about we use desired_pickler instead of pickle_lib to avoid possible confusion due to a typo?

It seems like the condition below should be inverted.

Let's change the function to: set_pickle_lib to avoid confusion with a usage that inverts a current value without taking desired value into account.

yes I like these suggestions.

tvalentyn · 2021-11-03T00:22:53Z

I think Run Unit Tests is not a valid trigger command, tests run by themselves when you push new changes. Github action tests don't have trigger commands, but could be re-triggered by via a UI. Jenkins suites have trigger commands listed in quotes and also in https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md . There is also "retestt this please" command that reruns all Jenkins suites (typo intentional to avoid actual re-test as I type this).

tvalentyn · 2021-11-03T00:26:56Z

Looks like there are a few errors in the suites:

2:32:13 > Task :sdks:python:test-suites:portable:py36:portableWordCountFlinkRunnerBatch FAILED
12:32:13 Traceback (most recent call last):
12:32:13   File "/usr/lib/python3.6/runpy.py", line 183, in _run_module_as_main
12:32:13     mod_name, mod_spec, code = _get_module_details(mod_name, _Error)
12:32:13   File "/usr/lib/python3.6/runpy.py", line 109, in _get_module_details
12:32:13     __import__(pkg_name)
12:32:13   File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Portable_Python_Commit/src/build/gradleenv/2022703440/lib/python3.6/site-packages/apache_beam/__init__.py", line 90, in <module>
12:32:13     import apache_beam.internal.pickler
12:32:13   File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Portable_Python_Commit/src/build/gradleenv/2022703440/lib/python3.6/site-packages/apache_beam/internal/pickler.py", line 31, in <module>
12:32:13     from apache_beam.internal import cloudpickle_pickler
12:32:13   File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Portable_Python_Commit/src/build/gradleenv/2022703440/lib/python3.6/site-packages/apache_beam/internal/cloudpickle_pickler.py", line 49, in <module>
12:32:13     import cloudpickle
12:32:13 ModuleNotFoundError: No module named 'cloudpickle'

For this one, you need to add a dependency on cloudpickle in setup.py

tvalentyn · 2021-11-03T00:28:35Z

sdks/python/container/base_image_requirements.txt

 # TODO(AVRO-2429): Upgrade to >= 1.9.0 only after resolved
 avro-python3==1.8.2
 fastavro==1.0.0.post1
+cloudpickle=2.0.0


needs == instead of = (see Docker suite failure).

aha. That's why it was failing. Done.

CHANGES.md

tvalentyn · 2021-11-30T23:54:25Z

sdks/python/container/py36/base_image_requirements.txt

 google-crc32c==1.3.0
 google-pasta==0.2.0
-google-python-cloud-debugger==2.18
+google-python-cloud-debugger==2.8


Was this an accidental manual change?

yeah there is something weird going on here. I've manually changed this PR to only add cloudpickle. It shouldn't be doing anything else.

added valentines suggestions. Co-authored-by: tvalentyn <tvalentyn@users.noreply.github.com>

…nging it back

ryanthompson591 added 7 commits September 7, 2021 15:09

wrapped pickler so that pickler is chosen

39a964d

Merge branch 'apache:master' into pickle-prototype

24b110a

changes to pickler

c1f8608

Update unit tests and correctly add global setter

954a46f

updated cloudpickle_pickler_test

9947720

added scope test

dfa434a

updated some comments

c8ff50c

ryanthompson591 changed the title ~~wrapped pickler so that pickler is chosen~~ Add cloudpickle as optional library Oct 29, 2021

ryanthompson591 added 2 commits October 29, 2021 12:14

Merge branch 'apache:master' into pickle-prototype

a6475e5

Allow absl library not to be present.

54d1c1b

allow multiple error types

36556fc

removed absl flags import

5d7de36

add cloudpickle dependency

9d5587b

tvalentyn reviewed Nov 3, 2021

View reviewed changes

ryanthompson591 added 8 commits November 4, 2021 12:31

Merge branch 'apache:master' into pickle-prototype

7c92db4

applied valentyn comments. remove dill dependencies

2d91d60

linted file

eaa17ab

change cloudpickle max requirement

9eed711

fixed pickle lib typo

62c7d0d

fixed pickle lib typo 2

c5a9df0

sets the default pickler

771235b

revert last change

64b74f4

ryanthompson591 and others added 18 commits November 23, 2021 14:07

updated base_image_requirements

a2012cb

Merge branch 'apache:master' into pickle-prototype

a9a9784

linted to remove space in default arg'

b078766

Added cloudpickle to requirements

db0c27e

linted

adb7ae4

change dill reference in coders

3400ef4

Merge branch 'apache:master' into pickle-prototype

8cde88c

only import lock if it can be imported otherwise ignore

e848b7a

Merge branch 'apache:master' into pickle-prototype

e57e987

linted tests removed unused variables changed line size

dd56672

moved imports, reverted file that wasnt changed

7fb9f53

changed import order

9f08887

trying a small fix

032c631

removed rlock pickling

92e3072

Merge branch 'apache:master' into pickle-prototype

1c66fc3

added to change file

94a9c96

Merge branch 'apache:master' into pickle-prototype

66c8069

Minor wording suggestion.

4ec71c3

tvalentyn approved these changes Nov 30, 2021

View reviewed changes

ryanthompson591 and others added 9 commits December 1, 2021 09:17

Merge branch 'apache:master' into pickle-prototype

f42051f

Merge branch 'apache:master' into pickle-prototype

915c5c6

merged

165c8d0

Update CHANGES.md

f7b2682

added valentines suggestions. Co-authored-by: tvalentyn <tvalentyn@users.noreply.github.com>

merged change.md changes

31aba48

merged

f20ce43

linted again

4f617b9

removed file that is also removed in head, not sure why git keeps bri…

4e0a330

…nging it back

removed changes that shouldnt be relevant to this pr

232ecac

tvalentyn merged commit bf9b959 into apache:master Dec 1, 2021

damccorm mentioned this pull request Jun 4, 2022

Make cloudpickle the default pickle library #21298

Closed

Conversation

ryanthompson591 commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

ryanthompson591 commented Oct 29, 2021

Uh oh!

ryanthompson591 commented Oct 29, 2021

Uh oh!

ryanthompson591 commented Oct 29, 2021

Uh oh!

ryanthompson591 commented Oct 29, 2021

Uh oh!

ryanthompson591 commented Oct 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tvalentyn Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tvalentyn Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

ryanthompson591 Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

tvalentyn commented Nov 3, 2021

Uh oh!

tvalentyn commented Nov 3, 2021

Uh oh!

tvalentyn Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

ryanthompson591 Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tvalentyn Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

ryanthompson591 Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryanthompson591 commented Sep 7, 2021 •

edited

Loading

`ValidatesRunner` compliance status (on master branch)