pandas-dev / pandas

Flexible and powerful data analysis / manipulation library for Python, providing labeled data structures similar to R data.frame objects, statistical functions, and much more
https://pandas.pydata.org
BSD 3-Clause "New" or "Revised" License
43.8k stars 17.98k forks source link

BUG: pd.eval with engine="numexpr" fails with simple expressions with float literals #59736

Open fdrocha opened 2 months ago

fdrocha commented 2 months ago

Pandas version checks

Reproducible Example

import pandas as pd
pd.eval("1/2", engine="numexpr")

Issue Description

This throws an exception

ValueError: Expression (np.float64(1.0)) / (np.float64(2.0)) has forbidden control characters.

Changing engine to "python" makes it work again. Strangely, replacing the division with any other operator also fixes things. I tried this in a clean venv with just the minimal packages installed.

This is similar to older #54542 but different. If I try to run the expression pandas is generating directly in numexpr I get the same error. Using sanitize=False is not enough to make it work however

import numexpr as ne
ne.evaluate("(np.float64(1.0)) / (np.float64(2.0))", sanitize=False)

This leads to a different exception: AttributeError: 'VariableNode' object has no attribute 'float64'.

Not sure if this is really a bug in pandas or numexpr. It seems related to numpy 2.0 as well.

Expected Behavior

It should just return 0.5.

Installed Versions

INSTALLED VERSIONS ------------------ commit : d9cdd2ee5a58015ef6f4d15c7226110c9aab8140 python : 3.11.7.final.0 python-bits : 64 OS : Darwin OS-release : 22.6.0 Version : Darwin Kernel Version 22.6.0: Mon Jun 24 01:25:37 PDT 2024; root:xnu-8796.141.3.706.2~1/RELEASE_X86_64 machine : x86_64 processor : i386 byteorder : little LC_ALL : None LANG : en_US LOCALE : en_US.UTF-8 pandas : 2.2.2 numpy : 2.1.1 pytz : 2024.1 dateutil : 2.9.0.post0 setuptools : 65.5.0 pip : 24.2 Cython : None pytest : None hypothesis : 6.112.0 sphinx : None blosc : None feather : None xlsxwriter : None lxml.etree : None html5lib : None pymysql : None psycopg2 : None jinja2 : None IPython : None pandas_datareader : None adbc-driver-postgresql: None adbc-driver-sqlite : None bs4 : None bottleneck : None dataframe-api-compat : None fastparquet : None fsspec : None gcsfs : None matplotlib : None numba : None numexpr : 2.10.1 odfpy : None openpyxl : None pandas_gbq : None pyarrow : None pyreadstat : None python-calamine : None pyxlsb : None s3fs : None scipy : None sqlalchemy : None tables : None tabulate : None xarray : None xlrd : None zstandard : None tzdata : None qtpy : None pyqt5 : None
Tunneller commented 2 months ago

I tried forcibly setting sanitize to False in necompiler.stringToExpression() which then reproduced the error AttributeError: 'VariableNode' object has no attribute 'float64'

so the problem is twofold: pandas is adding .float64 which numexpr doesnt want to see, and secondly the only keywords that numexpr is willing to tolerate are .real and .imag so .float64 breaks the sanitizer.

If you do the hack: s = s.replace("np.float64", "")

then pd.eval("1/2", engine="numexpr") works correctly.

rhshadrach commented 2 months ago

Thanks for the report. I can reproduce the OP on 2.2.x, but on main I am seeing 0.0 which is not expected. Further investigations and PRs to fix are welcome.

Tunneller commented 2 months ago

I'm happy to upload my hack to necompiler.py, but it really seems a hack....

The original code was def stringToExpression(s, types, context, sanitize: bool=True). I changed "s" to "ss" and put in the string replace statement. The problem goes away, or at least the symptom goes away. I think at some level there needs to be a discussion between Panda's and Numexpr guru's about what syntax they should be passing to each other.

def stringToExpression(ss, types, context, sanitize: bool=True):
    """Given a string, convert it to a tree of ExpressionNode's.
    """
    # sanitize the string for obvious attack vectors that NumExpr cannot 
    # parse into its homebrew AST. This is to protect the call to `eval` below.
    # We forbid `;`, `:`. `[` and `__`, and attribute access via '.'.
    # We cannot ban `.real` or `.imag` however...
    # We also cannot ban `.\d*j`, where `\d*` is some digits (or none), e.g. 1.5j, 1.j
    s = ss.replace("np.float64", "")
Tunneller commented 2 months ago

Alternatively, it looks like you could put something here on the pandas side in pandas/core/computation /eval.py but I have not attempted to validate this.

if engine == "numexpr" :
   parsed_expr  = parsed_expr.replace("np.float64","")
rhshadrach commented 1 month ago

@Tunneller - it seems to me removing text from parsed_expr could have unintended ill-effects, and is likely covering up the deeper issue.

Tunneller commented 1 month ago

Well, I assume the root cause is that Pandas decided to send explicit type information to numexpr and didn’t tell the numexpr guys.

Meanwhile Numexpr believes that the string “np.float64” is evidence of a seditious attempt to hack the eval operation. No surprise what happens next.

So I guess the deeper issue is communication, working in silos, etc. Nothing that can’t be solved. Out of my pay grade though.

In the mean-time, the only solution users have is to roll back to the previous pandas version, which seems a shame. Well, that or hack the expression with str.replace()

Regards, John

From: Richard Shadrach @.> Sent: Sunday, September 22, 2024 09:27 AM To: pandas-dev/pandas @.> Cc: John Lovell @.>; Mention @.> Subject: Re: [pandas-dev/pandas] BUG: pd.eval with engine="numexpr" fails with simple expressions with float literals (Issue #59736)

@Tunneller https://github.com/Tunneller - it seems to me removing text from parsed_expr could have unintended ill-effects, and is likely covering up the deeper issue.

— Reply to this email directly, view it on GitHub https://github.com/pandas-dev/pandas/issues/59736#issuecomment-2366812696 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMBZGK3FFLXB5FJQAKVUJDZX3HTPAVCNFSM6AAAAABNYW6B32VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRWHAYTENRZGY . You are receiving this because you were mentioned. https://github.com/notifications/beacon/AEMBZGPSC25XMCTZ66KQ74TZX3HTPA5CNFSM6AAAAABNYW6B32WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUNCKZBQ.gif Message ID: @. @.> >

rhshadrach commented 1 month ago

Well, I assume the root cause is that Pandas decided to send explicit type information to numexpr and didn’t tell the numexpr guys.

Meanwhile Numexpr believes that the string “np.float64” is evidence of a seditious attempt to hack the eval operation. No surprise what happens next.

Not sure why you state this, on main pandas is giving the result 0.0. The code is currently seeing that all scalars are integers and sets the return type to being an integer. We should consider the operations along with the scalars and when division is seen, the result should be float.

PRs to fix are welcome.

Tunneller commented 1 month ago

Completely bizarre. Maybe I had earlier version of panda??

On mine if you sent 4. / 9. to numexp eval, then be necompiler.py died with an Error about invalid string which was caused because only valid string operations defined should be “real” and “imag” whereas pandas had sent the string “np.float64 (4.) / np.float64 (9.)”

Maybe you have a different test case.

What version of Pandas and Numexpr do you have? I’ll install tomorrow.

John

On Sep 22, 2024, at 8:28 PM, Richard Shadrach @.***> wrote:  Well, I assume the root cause is that Pandas decided to send explicit type information to numexpr and didn’t tell the numexpr guys.

Meanwhile Numexpr believes that the string “np.float64” is evidence of a seditious attempt to hack the eval operation. No surprise what happens next.

Not sure why you state this, on main pandas is giving the result 0.0. The code is currently seeing that all scalars are integers and sets the return type to being an integer. We should consider the operations along with the scalars and when division is seen, the result should be float.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

rhshadrach commented 1 month ago

I am working off of the main branch with numexpr 2.10.1.

Tunneller commented 1 month ago

Hi, on pandas 2.2.2, then pd.eval("1.0/2.0") sent the expression (np.float64(1.0)) / (np.float64(2.0)) which caused numexpr to die. On pandas 2.2.3, then pd.eval("1.0/2.0") sends the expression (1.0) / (2.0) to numexpr and the world is a happy place. Basically pandas stripped out the offending np.float64 statement, as I had proposed, and everything runs fine now.

So I think the original post can be closed as fixed.

Tunneller commented 1 month ago

There is conceivably a different aspect which is that for integer division while pd.eval("1/2") now returns the correct value of zero, with no complaints, perhaps it should have returned a integer 0 rather than a float64 0.0

I think this is the issue that @rhshadrach found.

rhshadrach commented 1 month ago

I would expect 1/2 to give 0.5 as it does in Python, NumPy, and pandas.

print(1/2)
# 0.5
print(np.array([1]) / np.array([2]))
# [0.5]
print(pd.Series([1]) / pd.Series([2]))
# 0    0.5
# dtype: float64

Why do you think it should give 0?

Tunneller commented 1 month ago

Hi Richard, I had assumed it was integer division, but you are correct, that’s not quite how numexpr does it

import numexpr as ne

ne.evaluate("1/2")

array(0.5)

ne.evaluate("1./2.")

array(0.5)

ne.evaluate("1/2")

array(0.5)

ne.evaluate("1//2")

array(0, dtype=int32)

Meanwhile with pandas 2.2.3

import pandas as pd

pd.eval("1/2")

np.float64(0.0)

pd.eval("1./2.")

np.float64(0.5)

pd.eval("1//2")

np.int64(0)

pd.eval("21/9")

np.float64(2.0)

pd.eval("21//9")

np.int64(2)

So it seems like pd.eval( (int a) / (int b) ) somehow converts (int a )/(int b) into (float) [ (int a ) // ( int b )]. Not completely intuitive.

But either way, it appears to be a different bug from the one we had posted abut before a few weeks back. I think you can close the first one.

Regards, John

From: Richard Shadrach @.> Sent: Monday, September 23, 2024 07:55 PM To: pandas-dev/pandas @.> Cc: John Lovell @.>; Mention @.> Subject: Re: [pandas-dev/pandas] BUG: pd.eval with engine="numexpr" fails with simple expressions with float literals (Issue #59736)

I would expect 1/2 to give 0.5 as it does in Python, NumPy, and pandas.

print(1/2)

0.5

print(np.array([1]) / np.array([2]))

[0.5]

print(pd.Series([1]) / pd.Series([2]))

0 0.5

dtype: float64

Why do you think it should give 0?

— Reply to this email directly, view it on GitHub https://github.com/pandas-dev/pandas/issues/59736#issuecomment-2369884396 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMBZGLDWCNZ3JFI7RQCVH3ZYCZ7XAVCNFSM6AAAAABNYW6B32VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRZHA4DIMZZGY . You are receiving this because you were mentioned. https://github.com/notifications/beacon/AEMBZGJVXB35OMOYYK4XPZ3ZYCZ7XA5CNFSM6AAAAABNYW6B32WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUNIGIOY.gif Message ID: @. @.> >

rhshadrach commented 1 month ago

But either way, it appears to be a different bug from the one we had posted abut before a few weeks back. I think you can close the first one.

I am leaving this issue open because the example in the OP does not produce the correct result.

yuanx749 commented 1 month ago

I ran into the issue of pd.eval(1/2) = 0.0 and investigated a bit:

res is 0.5 below, which is correct, https://github.com/pandas-dev/pandas/blob/23c497bb2f7e05af1fda966e7fb04db942453559/pandas/core/computation/engines.py#L85-L86 but in reconstruct_object, typ is numpy.int64 and then typ(obj) becomes 0. https://github.com/pandas-dev/pandas/blob/23c497bb2f7e05af1fda966e7fb04db942453559/pandas/core/computation/align.py#L216