Tslibs offsets immutable

[jbrockmendel]

This is the big hurdle towards making DateOffsets immutable.

• Make tslibs.offsets._BaseOffset a cython class, define n and normalize to be readonly
• Update tests that try to alter offsets inplace
• Poke at things until pickle tests stop failing <-- the part with the kludge

I'm not wild about the hack in pickle_compat, but haven't found any other way to avoid TestCommon.test_pickle_v0_15_2 failing with:

cls = <class 'pandas.tseries.offsets.YearBegin'>, base = <type 'object'>, state = None

    def _reconstructor(cls, base, state):
        if base is object:
>           obj = object.__new__(cls)
E           TypeError: object.__new__(YearBegin) is not safe, use pandas._libs.tslibs.offsets._BaseOffset.__new__()

After this there will then be a) a bunch of other attributes to make readonly and b) the optimizations that are available once DateOffsets are immutable. But first we need to either OK the pickle_compat hack or find another way around.

[jreback]

no just define dunder reduce
you can’t use getstate/setstate in cython class

[jbrockmendel]

I'll try again with __reduce__. Don't hold your breath.

If we define __setstate__ in the non-cython class, trying to set self.n raises an AttributeError because it is readonly.

[jbrockmendel]

Isn't __reduce__ for pickling? The problem here is un-pickling legacy offsets.

[jreback]

this is not a feasible work around. you will have to look how I handled the ndarray subclassing case (this is similar).

[jbrockmendel]

this is not a feasible work around. you will have to look how I handled the ndarray subclassing case (this is similar).

OK, I'll keep poking at it. Where did the ndarray subclassing occur? Best guess is IndexOpsMixin?

[jbrockmendel]

Once we get #18224 sorted out, the assert n == int(n) will be done only once (and probably as a ValueError instead of assertion) in _BaseOffset.__init__

Update Woops, this comment belongs in #18226.

[jbrockmendel]

Will wrk be m this locally, get it off reviewers plate for now. Closing.

[jbrockmendel]

I'm not having much luck with this, have not given up. One idea: could we drop support for the 0.15.2 pickle? That would solve the current problem.

[jbrockmendel]

Implementing __reduce__ just leads to more errors. Can you expand a bit on what the problem is with the approach in this PR via pickle_compat?

[jbrockmendel]

@jreback suggestions on who I might be able to ask for help on this? Making offsets immutable is a pretty high priority for me (major PeriodIndex perf implications) and this is dead in the water.

[jreback]

@jbrockmendel not sure what is the specific issue

[jbrockmendel]

not sure what is the specific issue

TLDR: cython class + pickle + back-compat

We need to make DateOffsets immutable. To get there we need to change class _BaseOffset(object) to cdef class _BaseOffset(object). But doing that leads to failure in test_pickle_v0_15_2. The only workaround I've come up with got shot down.

[jreback]

I actually don’t have a problem with breaking pickles for offsets; you wooild rarely pickle these anyways

but i think u can still do this by using our substitution machinery - why don’t i give this a try

[jbrockmendel]

but i think u can still do this by using our substitution machinery - why don’t i give this a try

Awesome, go for it! My attempt to use the substitution machinery didn't work out, so with a little luck I'll learn something from seeing the results.

I actually don’t have a problem with breaking pickles for offsets

If the substitution approach doesn't work out, then after my remaining outstanding PRs are closed out I'll do it this way.

[jbrockmendel]

why don’t i give this a try

@jreback gotten a chance to look at this? I've been trying periodically and still haven't come up with anything other than the hack in pickle_compat here.

[WillAyd]

@jbrockmendel I couldn't pull down your branch locally (deleted?) but maybe this article helps:

https://stackoverflow.com/questions/4028321/any-ideas-about-the-best-work-around-for-new-losing-its-arguments

Namely that "You can't use object as the terminating class for your inheritance if you want to pass arguments to __new__ or __init__." - perhaps explicitly defining __new__ for _BaseOffset would help?

[jbrockmendel]

I couldn't pull down your branch locally (deleted?)

Yah, looks like I deleted the branch. I'll see if I can restore it without creating a new PR.

perhaps explicitly defining __new__ for _BaseOffset would help

Nope, cython raises a compile-time error telling you to use __cinit__ instead of __new__. If we do that, then in the tests we get errors in unpickling (see OP for description of the error and the kludged workaround). See @jreback's response disapproving and suggesting I look at [I never figured out exactly what]

[jbrockmendel]

@WillAyd the branch should be ~restored (made a new branch with the same name, copy/pasted the diff).

[WillAyd]

Still trying to recreate from the branch provided and haven't seen through yet but wondering at the moment if even just making a pass-through cinit would work, ala:

    def __cinit__(self, *args):
        pass

My current thinking there is a result of the below link:

http://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#initialisation-methods-cinit-and-init

Namely "If you anticipate subclassing your extension type in Python, you may find it useful to give the cinit() method * and ** arguments so that it can accept and ignore extra arguments. Otherwise, any Python subclass which has an init() with a different signature will have to override new() [1] as well as init(), which the writer of a Python class wouldn’t expect to have to do."

Doing such did resolve the TypeError you mentioned originally, though some other errors popped up I haven't had time to look at and may or may not have been resolved by subsequent work you did. Sharing for now in case this finding helps you at all

[jbrockmendel]

Still trying to recreate from the branch provided and haven't seen through yet

Thanks for taking a look. Note that when I put this together yesterday I got a test error in something like test_offsets.test_pickle_v_0_15_2 that wasn't present at the time of the PR. That is somewhat secondary, as The Problem is what happens when you revert the edits made in pickle_compat.

[WillAyd]

I don't get any errors when reverting the changes made in pickle_compat from the branch you remade

[jbrockmendel]

Not even in TestCommon.test_pickle_v0_15_2? I'm getting that error (py27, trying py36 shortly) regardless of whether the pickle_compat changes are reverted or not.

[WillAyd]

No errors at all running

pytest pandas/tests/io/test_pickle.py

This was using python3 so maybe that's the differentiator

[jbrockmendel]

Hmm I've fixed the v0_15_2 error I was getting (had mixed and matched diffs), but I'm still getting failures when I revert the pickle_compat edits (py27 and py36, cython 0.28.3).