bpo-44676: Add ability to serialise types.Union by uriyyo · Pull Request #27244 · python/cpython

uriyyo · 2021-07-19T16:56:08Z

https://bugs.python.org/issue44676

ambv · 2021-07-20T16:15:02Z

Lib/test/test_types.py

+        s = pickle.dumps(alias)
+        loaded = pickle.loads(s)
+        self.assertEqual(alias.__args__, loaded.__args__)
+        self.assertEqual(alias.__parameters__, loaded.__parameters__)


Those are good checks 👍🏻
I'd also put a plain self.assertEqual(alias, loaded). At the moment it's redundant with comparing __args__ but this might not be true later, so this will keep future-proof.

Thanks, I added self.assertEqual(alias, loaded) line to test.

ambv · 2021-07-20T16:16:23Z

Lib/test/test_types.py

+        ):
+            types.Union.from_args(1)
+
+        alias = types.Union.from_args((int, str, T))


At first I was a little weirded out by the single argument that is a tuple but looking at actual usage in typing.py this is the right choice 👍🏻

Using literals like in this test will be a rare occurence because then you can use the regular syntax for the union.

Agree, I used from_args only to test behaviour of this classmethod.

ambv · 2021-07-20T16:17:58Z

Objects/unionobject.c

 }

+static int
+is_args_unionable(PyObject *args)


Note to self: this is extracted from L450 below.

Objects/unionobject.c

uriyyo · 2021-07-20T16:57:29Z

@Fidget-Spinner I also cover case when args is empty tuple. In such case ValueError will be raised.

# Conflicts: # Lib/test/test_types.py # Objects/unionobject.c

bpo-44676: Add ability to serialize types.Union (pythonGH-27244)

Fidget-Spinner · 2021-07-24T13:24:12Z

Objects/unionobject.c

+        return NULL;
+    }
+
+    return Py_BuildValue("O(O)", from_args, alias->args);


@pablogsal and @uriyyo the reference leak is here, we need to decref from_args. I am submitting a PR now.

Yeah, I just catched it as well: #27332

Thanks @Fidget-Spinner for looking at it!

@Fidget-Spinner thanks for solving this issue

Fidget-Spinner · 2021-07-24T13:50:47Z

@uriyyo thanks again for fixing union pickling. I'm doing a manual backport without the reference leak now to keep 3.10 in sync.

(cherry picked from commit fe13f0b) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>

Fidget-Spinner · 2021-07-24T13:51:28Z

Nevermind it seems that Pablo got to it first :).

uriyyo · 2021-07-24T13:59:02Z

@Fidget-Spinner I just realise that Union._from_args will be removed by #27323. I think we should backport #27323 to not introduce Union._from_args at python 3.10.

…-27333) (cherry picked from commit fe13f0b) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>

Fidget-Spinner · 2021-07-24T14:10:46Z

@uriyyo I agree. We can backport this PR first though, because that PR and #27318 needs backporting which depends on code in this one. Then later on we can backport #27323 before 3.10rc1 on 2021-08-02.

bedevere-bot · 2021-07-24T14:42:57Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 macOS 3.10 has failed when building commit 9356d1e.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/681/builds/248) and take a look at the build logs.
Check if the failure is related to this commit (9356d1e) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/681/builds/248

Failed tests:

test_importlib

Failed subtests:

test_multiprocessing_pool_circular_import - test.test_importlib.test_threaded_import.ThreadedImportTests

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

408 tests OK.

10 slowest tests:

test_concurrent_futures: 3 min 37 sec
test_unparse: 2 min 45 sec
test_multiprocessing_spawn: 2 min 30 sec
test_tokenize: 2 min 18 sec
test_lib2to3: 1 min 55 sec
test_multiprocessing_forkserver: 1 min 45 sec
test_asyncio: 1 min 39 sec
test_unicodedata: 1 min 37 sec
test_capi: 1 min 31 sec
test_io: 58.5 sec

1 test failed:
test_importlib

18 tests skipped:
test_devpoll test_epoll test_gdb test_ioctl test_msilib
test_multiprocessing_fork test_ossaudiodev test_smtpnet test_spwd
test_ssl test_startfile test_tix test_tk test_ttk_guionly
test_winconsoleio test_winreg test_winsound test_zipfile64

Total duration: 31 min 9 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/test/test_importlib/test_threaded_import.py", line 258, in test_multiprocessing_pool_circular_import
    script_helper.assert_python_ok(fn)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/test/support/script_helper.py", line 160, in assert_python_ok
    return _assert_python(True, *args, **env_vars)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/test/support/script_helper.py", line 145, in _assert_python
    res.fail(cmd_line)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/test/support/script_helper.py", line 72, in fail
    raise AssertionError("Process return code is %d\n"
AssertionError: Process return code is 1
command line: ['/Users/buildbot/buildarea/3.10.billenstein-macos/build/python.exe', '-X', 'faulthandler', '-I', '/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/test/test_importlib/partial/pool_in_threads.py']


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/test/test_importlib/partial/pool_in_threads.py", line 9, in t
    with multiprocessing.Pool(1):
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/context.py", line 119, in Pool
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/pool.py", line 212, in __init__
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/pool.py", line 303, in _repopulate_pool
    return self._repopulate_pool_static(self._ctx, self.Process,
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/pool.py", line 326, in _repopulate_pool_static
    w.start()
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/process.py", line 121, in start
    self._popen = self._Popen(self)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/context.py", line 284, in _Popen
    return Popen(process_obj)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/popen_spawn_posix.py", line 32, in __init__
    super().__init__(process_obj)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/popen_fork.py", line 19, in __init__
    self._launch(process_obj)
  File "/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/popen_spawn_posix.py", line 54, in _launch
    child_r, parent_w = os.pipe()
OSError: [Errno 24] Too many open files
/Users/buildbot/buildarea/3.10.billenstein-macos/build/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 102 leaked semaphore objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '
---

…thonGH-27244) (pythonGH-27333)"

bpo-44676: Add ability to serialise types.Union

03e7227

uriyyo requested a review from gvanrossum as a code owner July 19, 2021 16:56

bedevere-bot added the awaiting review label Jul 19, 2021

the-knights-who-say-ni added the CLA signed label Jul 19, 2021

Fidget-Spinner self-requested a review July 19, 2021 17:04

ambv approved these changes Jul 20, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 20, 2021

Fidget-Spinner reviewed Jul 20, 2021

View reviewed changes

Objects/unionobject.c Outdated Show resolved Hide resolved

Make from_args protected

432ef67

uriyyo requested a review from Fidget-Spinner July 20, 2021 16:55

Merge remote-tracking branch 'upstream/main' into fix-issue-44676

3184435

# Conflicts: # Lib/test/test_types.py # Objects/unionobject.c

ambv merged commit fe13f0b into python:main Jul 23, 2021

bedevere-bot removed the awaiting merge label Jul 23, 2021

sthagen added a commit to sthagen/python-cpython that referenced this pull request Jul 23, 2021

Merge pull request #556 from python/main

5c60df4

bpo-44676: Add ability to serialize types.Union (pythonGH-27244)

uriyyo mentioned this pull request Jul 24, 2021

bpo-44676: Serialize the union type using only public API #27323

Merged

Fidget-Spinner reviewed Jul 24, 2021

View reviewed changes

This was referenced Jul 24, 2021

bpo-44676: Fix reference leaks in union_reduce #27332

Merged

bpo-44731: Simplify the union type implementation #27318

Merged

pablogsal pushed a commit to pablogsal/cpython that referenced this pull request Jul 24, 2021

[3.10] bpo-44676: Add ability to serialize types.Union (pythonGH-27244)

ab96d2f

(cherry picked from commit fe13f0b) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>

pablogsal added a commit that referenced this pull request Jul 24, 2021

[3.10] bpo-44676: Add ability to serialize types.Union (GH-27244) (GH…

9356d1e

…-27333) (cherry picked from commit fe13f0b) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>

Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Jul 24, 2021

Partially revert "bpo-44676: Add ability to serialize types.Union (py…

9e15f5a

…thonGH-27244) (pythonGH-27333)"

Uh oh!

Conversation

uriyyo commented Jul 19, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uriyyo commented Jul 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner commented Jul 24, 2021

Uh oh!

Fidget-Spinner commented Jul 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uriyyo commented Jul 24, 2021

Uh oh!

Fidget-Spinner commented Jul 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jul 24, 2021

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

uriyyo commented Jul 19, 2021 •

edited by bedevere-bot

Loading

Fidget-Spinner commented Jul 24, 2021 •

edited

Loading

Fidget-Spinner commented Jul 24, 2021 •

edited

Loading