PERF: fastpath DataFrame constructor from BlockManager

[jbrockmendel]

When trying to make fast_apply unnecessary I found that there is non-trivial overhead in the constructors that we can avoid. Moreover, since users shouldn't be passing BlockManagers around anyway, we might be able to get that case out of the DataFrame constructor entirely.

In [2]: ser = pd.Series(range(5))                                                                                                                                        
In [3]: df = ser.to_frame()                                                                                                                                              
In [4]: mgr = df._mgr

In [5]: %timeit pd.DataFrame(mgr)                                                                                                                                       
2.08 µs ± 51.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [6]: %timeit pd.DataFrame._from_mgr(mgr)                                                                                                                             
1.26 µs ± 17.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

[simonjayhawkins]

Moreover, since users shouldn't be passing BlockManagers around anyway, we might be able to get that case out of the DataFrame constructor entirely.

makes sense but would changing obj._constructor -> type(obj)._from_mgr create issues with subclassing. I recall a recent discussion that _constructor should be type(self)

[jbrockmendel]

makes sense but would changing obj._constructor -> type(obj)._from_mgr create issues with subclassing.

Fair point. If _constructor is something other than type(self) this would be change behavior. Will update.

I recall a recent discussion that _constructor should be type(self)

This sounds familiar. I'd be +1.

[simonjayhawkins]

can you add the return type

[jbrockmendel]

Would it be NDFrame or FrameOrSeries?

[simonjayhawkins]

I think

Suggested change

	def _from_mgr(cls, mgr: BlockManager):
	def _from_mgr(cls: Type[FrameOrSeries], mgr: BlockManager) -> FrameOrSeries:

though not checked

[jbrockmendel]

looks like we need to change from classmethod in order to get _constructor, unless we want to do something like

constructor = cls._constructor.fget(cls)

which seems sketchy

[simonjayhawkins]

does obj._constructor -> obj._constructor._from_mgr not work with the classmethod. obj._constructor returns a class type not an instance?

[jreback]

might consider calling finalize here (have to pass the method as an arg)

[jbrockmendel]

one of the use cases i have in mind is in groupby _chop where we are potentially creating many many Series/DataFrame objects, hoping we can avoid the extra call

[jorisvandenbossche]

Moreover, since users shouldn't be passing BlockManagers around anyway, we might be able to get that case out of the DataFrame constructor entirely.

This is however used by downstream libraries (eg pyarrow).

I recall a recent discussion that _constructor should be type(self)

That is currently not required to be the case (and for geopandas it also is not the case). After that earlier discussion, this issue was opened about that: #32638

[jorisvandenbossche]

I am not sure we can simply change away from using _constructor(that at least needs more investigation to check the impact on subclasses), or assume _constructor is a class (#32638)

But couldn't we provide a fast path in the DataFrame constructor that does more or less the same?
Basically check that data is a BlockManager and that all other keywords are None? (that are some more checks of course, so would need to check if that doesn't undo the gain)

[simonjayhawkins]

@jorisvandenbossche thanks for the link. you wrote

we also don't require _constructor to be a class, it can be a constructor function

so #33357 (comment) is probably not applicable.

[jorisvandenbossche]

The only requirement we have is that _constructor returns a callable, so obj._constructor._from_mgr will not necessarily work.

[jbrockmendel]

Basically check that data is a BlockManager and that all other keywords are None? (that are some more checks of course, so would need to check if that doesn't undo the gain)

if this or something like it isnt accepted, then that may be worthwhile. if we do get this fastpath, then id prefer not to make the already-complicated constructor more complicated.

The only requirement we have is that _constructor returns a callable, so obj._constructor._from_mgr will not necessarily work.

How about

def _from_mgr(self, mgr):
    constr = self._constructor
    if constr is type(self):
         [the current implementation]
    else:
        return constr(mgr)

[jorisvandenbossche]

How about

Then I personally think having the shortcut in the main constructor is the simpler solution

[jorisvandenbossche]

I am certainly +1 on optimizing the construction of a DataFrame from a BlockManager, but as mentioned before, I would rather do this in the main DataFrame constructor instead of adding _from_mgr (although I am generally fan of more specialized constructors):

• It will be a less invasive change, as we don't need to change _constructor into _from_mgr in all the places where you did it now
• Related to the above, it is also simpler: we just use _constructor, and don't need to remember to use _from_mgr instead of _constructor in certain cases (it is also simpler to understand, as it is not obvious apart from looking at its implementation that _from_mgr is still using _constructor for subclass-compatibility)
• Although it would add an extra check to DataFrame.__init__, it is not expanding the scope of the init, since it already accepts a BlockManager as argument right now
• Doing this in the main constructor ensures subclasses also benefit from the performance improvement.

[jbrockmendel]

On further reflection, I'm on board with Joris's suggestion. If at some point we add restrictions to _constructor that allow _from_mgr to be re-simplified, ill revisit at that point.

[jreback]

are there any places where we are actually setting fastpath=True now ? we should deprecate this

[jbrockmendel]

Series.take, DataFrame._box_col_values. I generally agree it would be nice to be rid of

[jorisvandenbossche]

And several more places as well.
But that's an independent discussion I would say

[jorisvandenbossche]

Thanks Brock!