CoW: add warning mode for cases that will change behaviour #55428

jorisvandenbossche · 2023-10-06T17:43:04Z

An initial PR working towards one of the remaining TODO items:

Add a warning mode that gives deprecation warnings for all cases where the current behaviour would change (initially also behind an option)

The current PR adds an option pd.options.mode.copy_on_write = "warn", and then everything behaves as the current default (without CoW enabled) but will raise a warning in case the behaviour will change if CoW would be enabled.

As a first step, this PR adds the option, and adds the warning in one specific code path that can trigger CoW, i.e. updating a Series (Manager.setitem_inplace). So this covers a typical case like:

s = df["col"]
s[0] = ..

no longer updating df.

In addition, I also turned off SettingWithCopyWarning when the CoW warning mode is enabled (because in that case, those warnings are no longer correct for the future behaviour, and it seemed better to not raise both kinds of warnings at the same time)

jorisvandenbossche · 2023-10-06T17:44:20Z

There are still a whole bunch of failures (with the warning mode enabled, for which I still need to add a CI build), but quite some of them are due to inplace operators (eg s += 1) unnecessarily triggering CoW, which is something I will try to tackle as a precursor.

phofl

first part of the review (wifi connection on the train isn't stable)

pandas/core/frame.py

pandas/core/internals/managers.py

phofl · 2023-10-14T16:31:57Z

pandas/core/internals/managers.py

+                if cow_context == "chained-assignment":
+                    warnings.warn(
+                        "ChainedAssignmentError: behaviour will change in pandas 3.0 "
+                        "with Copy-on-Write ...",


I think we need a doc page that we can link to that explains this more in-depth

phofl · 2023-10-14T16:43:43Z

pandas/core/series.py

                warnings.warn(
                    _chained_assignment_msg, ChainedAssignmentError, stacklevel=2
                )
+            elif warn_copy_on_write() and sys.getrefcount(self) <= 3:
+                cow_context = "chained-assignment"


I think you need to warn here instead of deferring. This might now warn, if the first op in chained assignment created a copy, e.g.

pd.options.mode.copy_on_write = "warn" df = pd.DataFrame({"a": [1, 2, 3,], "b": 1}) indexer = df.a > 1 df = df.a df[indexer][0] = 5

I think you could make a case for not warning here since the current behaviour is a no-op as well, but

the DataFrame case warns

I would prefer a warning since cow will also warn

So the reason that I didn't yet warn here and deferred it to Manager.setitem_inplace, is because I wanted the warning to be more specific, so we can distinguish the case of:

df = .. ser = df["col"] ser[0] = 0

-> here we want to warn that you are setting into a series that is a view of another, and that this change will no longer propagate (in this case no longer update the dataframe)

df = ... df["col"][0] = 0

-> here we want to warn that this simply won't work anymore in the future, as this is chained assignment (and the case of it that currently does work and is used widely I think, so I want to have a distinct warning for this one)

Now, while writing this I am not sure anymore why this alone warrants deferring it. I have to check in the tests for the case that made me move this to the lower level (I originally was just warning here, as you suggest, but then changed it)

phofl · 2023-10-14T16:51:35Z

pandas/tests/copy_view/test_indexing.py

-        warn = SettingWithCopyWarning if single_block else None
+        # TODO
+        warn = (
+            SettingWithCopyWarning


I am confused, shouldn't this be silent when CoW is enabled as well?

Yes, and that using_copy_on_Write case is already handled in the if block above, so in this else I still need to handle the warn_copy_on_write case.

I could actually do a elif warn_copy_on_write separate block, that might be easier to read and which is also the pattern I started to use in many other places.

pandas/tests/copy_view/test_indexing.py

pandas/tests/indexing/multiindex/test_setitem.py

pandas/tests/indexing/test_chaining_and_caching.py

jorisvandenbossche · 2023-10-14T20:30:42Z

@phofl I removed the last commit about the chained assignments again, and moved that to a separate PR (#55522). So this PR then just focuses on adding the infrastructure, and the warnings for (non-chained) series setitem (the things going through setitem_inplace). Then once this is merged, I can work more easily in parallel on different areas.

jorisvandenbossche · 2023-10-14T20:46:18Z

pandas/core/internals/managers.py

+COW_WARNING_SETITEM_MSG = """\
+Setting a value on a view: behaviour will change in pandas 3.0.
+Currently, the mutation will also have effect on the object that shares data
+with this object. For example, when setting a value in a Series that was
+extracted from a column of a DataFrame, that DataFrame will also be updated:
+
+    ser = df["col"]
+    ser[0] = 0     <--- in pandas 2, this also updates `df`
+
+In pandas 3.0 (with Copy-on-Write), updating one Series/DataFrame will never
+modify another, and thus in the example above, `df` will not be changed.
+"""


One question is: how elaborate do we want to make this warning message?

One the one hand, I think one can argue that it can be short and point to a doc page where it is explained in more detail (which I actually should add anyway).

On the other hand, I also want it to be understandable for the many people that won't read the docs about this in detail.

I would anticipate users also wanting to remove/address this warning, so in light of that, I would opt to move the explanation to a doc page if we can include potential actions users can take to remove this warning (e.g. set the copy-on-write mode to True)

Is it OK to leave this as is, for now, and do a separate PR to write the documentation and update the warnings (add links to the docs, potentially shorten it)?

Yeah sure thing 👍

Agree with the doc page

jorisvandenbossche · 2023-10-14T20:57:43Z

pandas/tests/copy_view/test_indexing.py

+        if warn_copy_on_write:
+            with tm.assert_produces_warning(FutureWarning):
                s[0] = 0


How important do we find it that we ensure the exact message? (given that this is only temporary, maybe not super important?)

(although I suppose it is not difficult to just add match="Setting a value on a view" to all cases I am adding in this PR.

I don't know how useful it will be to add a custom tm.assert_produces_cow_warning, but it would make the tm.assert_produces_warning(FutureWarning, match="Setting a value on a view"): a bit shorter.

But when adding warnings for more cases, the exact warning message might also differ.

I think it's okay being lax with matching the warning message during testing (but noting a comment that it's CoW related if it's not obvious from the test)

but noting a comment that it's CoW related if it's not obvious from the test

That might be a reason to actually make something like a tm.assert_produces_cow_warning, then it's also clear without adding a comment everywhere, and will be easier to remove all of those.

jorisvandenbossche · 2023-10-27T15:48:46Z

I added a tm.assert_cow_warning helper function to use for all the cases where specifically checking for this warning.

@phofl @mroeschke if possible, I would like to get this merged shortly if possible, as there is a lot more work to do here, but that will be easier done in parallel once this base PR is in.

phofl · 2023-10-27T17:03:36Z

I’ll check later today or tomorrow

phofl · 2023-10-30T10:59:30Z

thx @jorisvandenbossche

CoW: add warning mode for cases that will change behaviour

d9aefbf

jorisvandenbossche added the Copy / view semantics label Oct 6, 2023

jorisvandenbossche requested a review from phofl October 6, 2023 17:43

jorisvandenbossche added 2 commits October 13, 2023 18:19

add CI build

5f00be7

avoid warning with inplace operators

55b483b

phofl reviewed Oct 14, 2023

View reviewed changes

jorisvandenbossche added 2 commits October 14, 2023 22:17

update chained assignment tests to silence current failures

7836601

address feedback

e16abb4

jorisvandenbossche force-pushed the cow-warnings branch from d3f53c0 to e16abb4 Compare October 14, 2023 20:24

jorisvandenbossche mentioned this pull request Oct 14, 2023

CoW warning mode: warn in case of chained assignment #55522

Merged

jorisvandenbossche added this to the 2.2 milestone Oct 14, 2023

update warning message

4bf974c

jorisvandenbossche marked this pull request as ready for review October 14, 2023 20:43

jorisvandenbossche requested a review from mroeschke as a code owner October 14, 2023 20:43

jorisvandenbossche commented Oct 14, 2023

View reviewed changes

jorisvandenbossche mentioned this pull request Oct 14, 2023

CLN/TST: clean logic of old datetime test_indexing test #55523

Merged

jorisvandenbossche added 5 commits October 17, 2023 10:30

Merge remote-tracking branch 'upstream/main' into cow-warnings

ba6709c

fixup tests for changed warning text

92c0898

fix COW setitem

07dfbec

Merge remote-tracking branch 'upstream/main' into cow-warnings

6e9f3ff

add tm.assert_cow_warning and use everywhere

f7fa78e

mroeschke approved these changes Oct 27, 2023

View reviewed changes

phofl approved these changes Oct 30, 2023

View reviewed changes

phofl merged commit 192a4e3 into pandas-dev:main Oct 30, 2023
33 of 34 checks passed

jorisvandenbossche deleted the cow-warnings branch November 5, 2023 22:14

jorisvandenbossche mentioned this pull request Nov 5, 2023

CoW warning mode: add warning for single block setitem #55838

Merged

jorisvandenbossche mentioned this pull request Nov 17, 2023

CoW warning mode for cases that will change behaviour #56019

Open

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CoW: add warning mode for cases that will change behaviour #55428

CoW: add warning mode for cases that will change behaviour #55428

jorisvandenbossche commented Oct 6, 2023 •

edited

Loading

jorisvandenbossche commented Oct 6, 2023

phofl left a comment

phofl Oct 14, 2023

phofl Oct 14, 2023

jorisvandenbossche Oct 14, 2023

phofl Oct 14, 2023

jorisvandenbossche Oct 14, 2023

jorisvandenbossche commented Oct 14, 2023

jorisvandenbossche Oct 14, 2023

mroeschke Oct 15, 2023

jorisvandenbossche Oct 27, 2023

mroeschke Oct 27, 2023

phofl Oct 30, 2023

jorisvandenbossche Oct 14, 2023

mroeschke Oct 15, 2023

jorisvandenbossche Oct 17, 2023

jorisvandenbossche commented Oct 27, 2023 •

edited

Loading

phofl commented Oct 27, 2023

phofl commented Oct 30, 2023

CoW: add warning mode for cases that will change behaviour #55428

CoW: add warning mode for cases that will change behaviour #55428

Conversation

jorisvandenbossche commented Oct 6, 2023 • edited Loading

jorisvandenbossche commented Oct 6, 2023

phofl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 27, 2023 • edited Loading

phofl commented Oct 27, 2023

phofl commented Oct 30, 2023

jorisvandenbossche commented Oct 6, 2023 •

edited

Loading

jorisvandenbossche commented Oct 27, 2023 •

edited

Loading