classification
Title: [uuid] 3.8 breaks weak references for UUIDs
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Nir Soffer, dheiberg, josh.r, serhiy.storchaka, taleinat, vstinner, wbolster
Priority: Keywords: easy, patch, patch, patch

Created on 2019-01-09 17:37 by josh.r, last changed 2019-01-28 09:32 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11570 merged dheiberg, 2019-01-15 17:42
PR 11570 merged dheiberg, 2019-01-15 17:42
PR 11570 merged dheiberg, 2019-01-15 17:42
PR 11621 merged dheiberg, 2019-01-19 16:11
PR 11621 merged dheiberg, 2019-01-19 16:11
PR 11621 merged dheiberg, 2019-01-19 16:11
Messages (19)
msg333338 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-01-09 17:37
I 100% agree with the aim of #30977 (reduce uuid.UUID() memory footprint), but it broke compatibility for any application that was weak referencing UUID instances (which seems a reasonable thing to do; a strong reference to a UUID can be stored in a single master container or passed through a processing pipeline, while also keying WeakKeyDictionary with cached supplementary data). I specifically noticed this because I was about to do that very thing in a processing flow, then noticed UUIDs in 3.6 were a bit heavyweight, memory-wise, went to file a bug on memory usage to add __slots__, and discovered someone had already done it for me.

Rather than break compatibility in 3.8, why not simply include '__weakref__' in the __slots__ listing? It would also remove the need for a What's New level description of the change, since the description informs people that:

1. Instances can no longer be weak-referenced (which adding __weakref__ would undp)
2. Instances can no longer add arbitrary attributes. (which was already the case in terms of documented API, programmatically enforced via a __setattr__ override, so it seems an unnecessary thing to highlight outside of Misc/NEWS)

The cost of changing __slots__ from:

    __slots__ = ('int', 'is_safe')

to:

    __slots__ = 'int', 'is_safe', '__weakref__'

would only be 4-8 bytes (for 64 bit Python, total cost of object + int would go from 100 to 108 bytes, still about half of the pre-__slots__ cost of 212 bytes), and avoid breaking any code that might rely on being able to weak reference UUIDs.

I've marked this as release blocker for the time being because if 3.8 actually releases with this change, it will cause back compat issues that might prevent people relying on UUID weak references from upgrading their code.
msg333630 - (view) Author: David Heiberg (dheiberg) * Date: 2019-01-14 18:06
Since there has been no objection to this yet, would it be alright for me to take this as my first PR? 

On top of the change you mentioned to the __slots__ list, should there also be a test written so that a similar regression doesn't happen again?
msg333631 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-01-14 18:10
Indeed, a PR for this should include a test that weakrefs work.
msg333632 - (view) Author: wouter bolsterlee (wbolster) * Date: 2019-01-14 18:12
the test could be sth like

x = uuid.uuid4()
y = weakref.ref(x)
assert x is y()
msg333633 - (view) Author: David Heiberg (dheiberg) * Date: 2019-01-14 18:17
Ok thanks for your input, I will work on a PR and hopefully submit one tomorrow or Wednesday depending on schedule.
msg333818 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-01-17 04:15
David, the What's New note about weak references no longer being possible should be removed as part of this change. I'm not sure the note on arbitrary attributes no longer being addable is needed either (__setattr__ blocked that beforehand, it's just even more impossible now).
msg333852 - (view) Author: David Heiberg (dheiberg) * Date: 2019-01-17 11:35
Ahh yes of course, I will remove that from the notes. With regards to the second note, would it do any harm to leave it in? I suppose it does imply that this was possible before...
msg333855 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-17 12:16
New changeset f1d8e7cf17a010d2657822e06a41b30c9542a8c7 by Victor Stinner (David H) in branch 'master':
bpo-35701: Added __weakref__ slot to uuid.UUID (GH-11570)
https://github.com/python/cpython/commit/f1d8e7cf17a010d2657822e06a41b30c9542a8c7
msg333857 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-17 12:20
I understand that the reported issue is now fixed, so I close the issue.

Note: uuid.UUID of Python 3.7 doesn't use slots.
msg333858 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-17 12:29
What's New changes still are needed.

Are weak references to UUID used in any existing code, or just in the code that is planned to be written?
msg333859 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-17 12:31
> What's New changes still are needed.

Sorry, what do you want to document? The fact that uuid.UUID now use slots? If yes, maybe reopen bpo-30977 instead?

This issue is only about fixing a regression caused by bpo-30977 (it wasn't possible to create a weak ref to a UUI), no?
msg333860 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-17 12:34
This was already documented. We need to fix the documentation. See the discussion above.
msg333863 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-17 12:42
https://docs.python.org/dev/whatsnew/3.8.html#optimizations

"""uuid.UUID now uses __slots__ to reduce its memory footprint. Note that this means that instances can no longer be weak-referenced and that arbitrary attributes can no longer be added to them."""

Oh ok, now I understand previous comments. Yeah, the note should be removed.
msg333864 - (view) Author: David Heiberg (dheiberg) * Date: 2019-01-17 12:45
Should the note on arbitrary attributes also be removed? If this was documented previously then I don't see the need for it here, but if this has never been documented then maybe some other way of wording it may be sensible to include?
msg333929 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-01-18 01:51
The UUID module documentation (and docstring) begin with:

"This module provides immutable UUID objects"

Immutable is a stronger guarantee than __slots__ enforces already, so the documentation already ruled out adding arbitrary attributes to UUID (and the __setattr__ that unconditionally raised TypeError('UUID objects are immutable') supported that.

Given the behavior hasn't changed in any way that contradicts the docs, nor would it affect anyone who wasn't intentionally working around the __setattr__ block, I don't feel a need to mention the arbitrary attribute limitation.

It's fine to leave in the What's New note (it is a meaningful memory savings for applications using lots of UUIDs), but the note can simplify to just:

"""uuid.UUID now uses __slots__ to reduce its memory footprint."""
msg333991 - (view) Author: David Heiberg (dheiberg) * Date: 2019-01-18 16:53
Agreed. I will fix the documentation and submit a PR this weekend hopefully.
msg334454 - (view) Author: David Heiberg (dheiberg) * Date: 2019-01-28 09:11
I have submitted a PR for the documentation. Hopefully this is enough to resolve the issue and close.
msg334455 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-28 09:31
New changeset ea446409cd5f1364beafd5e5255da6799993f285 by Victor Stinner (David H) in branch 'master':
bpo-35701: Update doc for UUID weak referencing (GH-11621)
https://github.com/python/cpython/commit/ea446409cd5f1364beafd5e5255da6799993f285
msg334456 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-28 09:32
I merged the doc change. I understand that the issue can now be closed, thanks to everyone who helped here!
History
Date User Action Args
2019-01-28 09:32:53vstinnersetstatus: open -> closed
priority: release blocker ->
messages: + msg334456

keywords: patch, patch, patch, easy
resolution: fixed
stage: patch review -> resolved
2019-01-28 09:31:29vstinnersetmessages: + msg334455
2019-01-28 09:11:08dheibergsetmessages: + msg334454
2019-01-19 16:11:29dheibergsetstage: needs patch -> patch review
pull_requests: + pull_request11376
2019-01-19 16:11:20dheibergsetstage: needs patch -> needs patch
pull_requests: + pull_request11375
2019-01-19 16:11:08dheibergsetstage: needs patch -> needs patch
pull_requests: + pull_request11374
2019-01-18 16:53:11dheibergsetmessages: + msg333991
2019-01-18 01:51:19josh.rsetkeywords: patch, patch, patch, easy

messages: + msg333929
2019-01-17 12:45:53dheibergsetmessages: + msg333864
2019-01-17 12:42:13vstinnersetkeywords: patch, patch, patch, easy

messages: + msg333863
2019-01-17 12:34:45serhiy.storchakasetkeywords: patch, patch, patch, easy

messages: + msg333860
2019-01-17 12:31:48vstinnersetkeywords: patch, patch, patch, easy

messages: + msg333859
2019-01-17 12:29:37serhiy.storchakasetstatus: closed -> open
messages: + msg333858

keywords: patch, patch, patch, easy
resolution: fixed -> (no value)
stage: resolved -> needs patch
2019-01-17 12:20:14vstinnersetstatus: open -> closed
messages: + msg333857

keywords: - 3.7regression
resolution: fixed
stage: patch review -> resolved
2019-01-17 12:16:54vstinnersetmessages: + msg333855
2019-01-17 11:35:03dheibergsetmessages: + msg333852
2019-01-17 04:15:15josh.rsetkeywords: patch, patch, patch, easy, 3.7regression

messages: + msg333818
2019-01-15 17:42:47dheibergsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request11238
2019-01-15 17:42:35dheibergsetkeywords: + patch
stage: needs patch -> needs patch
pull_requests: + pull_request11237
2019-01-15 17:42:22dheibergsetkeywords: + patch
stage: needs patch -> needs patch
pull_requests: + pull_request11236
2019-01-14 18:17:03dheibergsetmessages: + msg333633
2019-01-14 18:12:08wbolstersetmessages: + msg333632
2019-01-14 18:10:48taleinatsetmessages: + msg333631
2019-01-14 18:06:45dheibergsetnosy: + dheiberg
messages: + msg333630
2019-01-10 17:57:24brett.cannonsettitle: 3.8 needlessly breaks weak references for UUIDs -> [uuid] 3.8 breaks weak references for UUIDs
2019-01-09 17:37:04josh.rcreate