[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Raymond Hettinger]

Raymond Hettinger  added the comment:

Given that this is rare and that it is a behavior change, I'm only applying 
this to 3.10.  If you think it is essential for 3.9, feel free to reopen and 
we'll discuss it with the release manager.

--
components: +Library (Lib)
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.10 -Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Raymond Hettinger]

Raymond Hettinger  added the comment:


New changeset 1e27b57dbc8c1b758e37a531487813aef2d111ca by masklinn in branch 
'master':
bpo-42470: Do not warn on sequences which are also sets in random.sample() 
(GH-23665)
https://github.com/python/cpython/commit/1e27b57dbc8c1b758e37a531487813aef2d111ca


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Xavier Morel]

Xavier Morel  added the comment:

Tried patterning the PR after the one which originally added the warning. 
Wasn't too sure how the news item was supposed to be generated, and grepping 
the repository didn't reveal any clear script doing that, so I made up a date 
and copied an existing random bit (which I expect is just a deduplicator in 
case multiple NEWS items are created at the same instant?)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Xavier Morel]

Change by Xavier Morel :


--
pull_requests: +22532
pull_request: https://github.com/python/cpython/pull/23665

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Roundup Robot]

Change by Roundup Robot :


--
keywords: +patch
nosy: +python-dev
nosy_count: 4.0 -> 5.0
pull_requests: +22507
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/23639

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Open it against master.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Xavier Morel]

Xavier Morel  added the comment:

I was preparing to open the PR but now I'm doubting: should I open the PR 
against master and miss islington will backport it, or should I open the PR 
against 3.9?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Raymond Hettinger]

Raymond Hettinger  added the comment:

>>> Do you want to submit a PR for this?
>
> Sure. Do you think the code I proposed would be suitable?

Yes.  It will need tests and a news entry as well.

--
assignee:  -> rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Xavier Morel]

Xavier Morel  added the comment:

> Do you want to submit a PR for this?

Sure. Do you think the code I proposed would be suitable?

> * The current logic matches the logic before the warning was added.
> * The proposed logic matches what the code will do after the
>   deprecation period (it will only check for non-sequences).

Yes, that was my basis for it as it seemed sensible, but you're right that it's 
a bit of a behavioural change as you note:

> * There is some value in the warning in that it lets you know an
>   inefficient code path is being used (i.e. the conversion to a tuple).
> * The proposed logic doesn't just change the warning, it changes
>   what actually happens to the data.  IMO the change is for the
>   better, but it is a behavior change and could potentially cause
>   a failure in someone's code.

Aye, and also I guess the "sequence" implementation of the input collection 
might be less efficient than one-shot converting to a set and sampling from the 
set.

> * The case of an object that is both a sequence and a set is
>   likely very rare.

Chances are you're right, but it's what got me to stumble upon it ($dayjob 
makes significant use of a "unique list / ordered set" smart-ish collection, 
that collection was actually registered against Set and Sequence specifically 
because Python 3's random.sample typechecks those, we registered against both 
as the collection technically implements both interfaces so that seemed like a 
good idea at the time).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Raymond Hettinger]

Raymond Hettinger  added the comment:

+0 

Do you want to submit a PR for this?

Some thoughts:

* The current logic matches the logic before the warning was added.
* The proposed logic matches what the code will do after the
  deprecation period (it will only check for non-sequences).
* There is some value in the warning in that it lets you know an
  inefficient code path is being used (i.e. the conversion to a tuple).
* The proposed logic doesn't just change the warning, it changes
  what actually happens to the data.  IMO the change is for the
  better, but it is a behavior change and could potentially cause
  a failure in someone's code.
* The case of an object that is both a sequence and a set is
  likely very rare.

--
nosy: +tim.peters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42470] DeprecationWarning triggers for sequences which happen to be sets as well

[Xavier Morel]

New submission from Xavier Morel :

In 3.9, using `random.sample` on sets triggers

DeprecationWarning: Sampling from a set deprecated
since Python 3.9 and will be removed in a subsequent version.

*However* it also triggers on types which implement *both* Sequence and Set, 
despite Sequence on its own being fine.

The issue is that it first checks for Set and triggers a warning, and only then 
checks that the input is a sequence:

if isinstance(population, _Set):
_warn('Sampling from a set deprecated\n'
  'since Python 3.9 and will be removed in a subsequent 
version.',
  DeprecationWarning, 2)
population = tuple(population)
if not isinstance(population, _Sequence):
raise TypeError("Population must be a sequence.  For dicts or sets, 
use sorted(d).")

the check should rather be:

if not isinstance(population, _Sequence):
if isinstance(population, _Set):
_warn('Sampling from a set deprecated\n'
  'since Python 3.9 and will be removed in a subsequent 
version.',
  DeprecationWarning, 2)
population = tuple(population)
else:
raise TypeError("Population must be a sequence.  For dicts or 
sets, use sorted(d).")

this also only incurs a single instance check for `_Sequence` types instead of 
two.

--
messages: 381885
nosy: xmorel
priority: normal
severity: normal
status: open
title: DeprecationWarning triggers for sequences which happen to be sets as well
versions: Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com