Skip to content

BUG: weird interaction between pyslurm, ujson that changes function signature of ujson.dumps #33877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
2 of 3 tasks
igozali opened this issue Apr 29, 2020 · 8 comments
Open
2 of 3 tasks
Labels
Bug IO JSON read_json, to_json, json_normalize

Comments

@igozali
Copy link

igozali commented Apr 29, 2020

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pandas.

  • (optional) I have confirmed this bug exists on the master branch of pandas.


Hi maintainers, I'd like to start with saying I'm not entirely sure if this is a pandas bug, but seeing that pandas bundles its own ujson, figured I'd ask here.

Problem description

TL;DR: There seems to be a very strange behavior in ujson depending on how it is imported (in particular, depending on whether the pyslurm library is imported first). The observed behavior is that this modifies the function signature of ujson.dumps (by removing the sort_keys argument), as shown in the below examples. I'm not sure if this behavior is specific to importing pyslurm in the beginning or not, but this is just one specific example that can be used to reproduce the problem.

First, installation of these libraries via pip is needed:

pip install cython
pip install pyslurm pandas ujson

This is the script with the error:

import pyslurm
import pandas
import ujson
print(f"pyslurm=={pyslurm.__version__}")
print(f"pandas=={pandas.__version__}")
print(f"ujson=={ujson.__version__}")
ujson.dumps({}, sort_keys=True)

If you run the script above, you'll see the output is:

pyslurm==18.08.1.1
pandas==1.0.3
ujson==2.0.3
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: 'sort_keys' is an invalid keyword argument for this function

However, if you remove the pyslurm import, the problem goes away!

import pandas
import ujson
print(f"pandas=={pandas.__version__}")
print(f"ujson=={ujson.__version__}")
ujson.dumps({}, sort_keys=True)

Also, reordering the imports somehow fixes the problem:

import pandas
import ujson
import pyslurm
ujson.dumps({}, sort_keys=True)

Expected Output

Importing pyslurm and pandas shouldn't affect the behavior of my ujson library. Also, import orders shouldn't cause this bug.

Additional Info

It seems that the very specific import order that causes this is pyslurm -> pandas -> ujson.

I suspect this has something to do with Python's mechanism for importing C extensions, but I don't understand the internals enough to be able to debug this further.

Output of pd.show_versions()

INSTALLED VERSIONS

commit : None
python : 3.8.2.final.0
python-bits : 64
OS : Linux
OS-release : 4.15.0-43-generic
machine : x86_64
processor : x86_64
byteorder : little
LC_ALL : None
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8

pandas : 1.0.3
numpy : 1.18.3
pytz : 2020.1
dateutil : 2.8.1
pip : 20.0.2
setuptools : 46.1.3.post20200330
Cython : 0.29.17
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : None
IPython : None
pandas_datareader: None
bs4 : None
bottleneck : None
fastparquet : None
gcsfs : None
lxml.etree : None
matplotlib : None
numexpr : None
odfpy : None
openpyxl : None
pandas_gbq : None
pyarrow : None
pytables : None
pytest : None
pyxlsb : None
s3fs : None
scipy : None
sqlalchemy : None
tables : None
tabulate : None
xarray : None
xlrd : None
xlwt : None
xlsxwriter : None
numba : None

@igozali igozali added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Apr 29, 2020
@jbrockmendel jbrockmendel added IO JSON read_json, to_json, json_normalize and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 6, 2020
@igozali
Copy link
Author

igozali commented Jul 24, 2020

@jbrockmendel do you think it's worth it to also ping the pyslurm maintainers to see what they think about this? Quite perplexed...

@jbrockmendel
Copy link
Member

@WillAyd might have a better idea on this one

@WillAyd
Copy link
Member

WillAyd commented Jul 25, 2020

I am not familiar with pyslurm at all so can't really speak to that, but my guess is that this may be a result of ujson (and potentially pyslurm) still using single phase module initialization. We switched our internal ujson implementation over to multi-phase in #30805 but ujson itself on master still uses single phase

You can read more about the differences in PEP 489

@igozali
Copy link
Author

igozali commented Jul 27, 2020

Thanks @WillAyd, that's very helpful indeed, I suspected it might have something to do with the intricacies of Python loading C extensions in some way but I didn't even know what to Google.

I saw that pyslurm uses Cython (minimum version 0.19) and according to this SO post, multi-phase initialization seems to have been enabled since 0.29 (although maybe the wheels might've been built with an older Cython version?).

Seems like it shouldn't be that difficult to convert upstream ujson to use multi-phase initialization? If that's a correct assessment somewhat maybe I might give it a shot to do so if that seems like something worth doing?

@WillAyd
Copy link
Member

WillAyd commented Jul 27, 2020

Cool! Yea I think for ujson you can pretty much do what was done in #30805 but on that project. After being inactive for quite a few years, there is a new maintainer of that who should be able to help get that in and released

If building pyslurm for sure try having a newer Cython version. If they distribute a wheel you could also ask those maintainers to do so during the wheel building process

@igozali
Copy link
Author

igozali commented Aug 2, 2020

Thanks @WillAyd I was about to try it out right this moment and realized the new version is out already :D

@igozali
Copy link
Author

igozali commented Aug 2, 2020

Unfortunately, I just tried it out with the new ujson==3.1.0, and the problem still seems to exist. I used this Docker image to reproduce it.

docker run -it giovtorres/docker-centos7-slurm:18.08.5 bash

# Inside the Docker image
python3.6 -m venv venv
. venv/bin/activate
pip install cython
pip install pyslurm pandas ujson

# Made sure to use a pretty recent Cython too (to take advantage of the multi-phase initialization)
(venv) [root@9f60ea63ab11 /]# pip freeze | grep -i cython
You are using pip version 10.0.1, however version 20.2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Cython==0.29.21

# On the Python interpreter
(venv) [root@9f60ea63ab11 /]# python
Python 3.6.7 (default, Dec  5 2018, 15:02:05)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-36)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyslurm
>>> import pandas
>>> import ujson
>>> print(f"pyslurm=={pyslurm.__version__}")
pyslurm==18.08.1.1
>>> print(f"pandas=={pandas.__version__}")
pandas==1.1.0
>>> print(f"ujson=={ujson.__version__}")
ujson==3.1.0
>>> ujson.dumps({}, sort_keys=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'sort_keys' is an invalid keyword argument for this function

# Excluding pyslurm import seems to still "fix" the problem
(venv) [root@9f60ea63ab11 /]# python
Python 3.6.7 (default, Dec  5 2018, 15:02:05)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-36)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pandas
>>> import ujson
>>> print(f"pandas=={pandas.__version__}")
pandas==1.1.0
>>> print(f"ujson=={ujson.__version__}")
ujson==3.1.0
>>> ujson.dumps({}, sort_keys=True)
'{}'

I wonder if this could be a bug on the Python interpreter side, even?

@jbrockmendel
Copy link
Member

We've changed the name from dumps to ujson_dumps. Might that solve this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IO JSON read_json, to_json, json_normalize
Projects
None yet
Development

No branches or pull requests

3 participants