Skip to content
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

Astropy on pypy ? #7768

Open
cdeil opened this issue Aug 24, 2018 · 13 comments
Open

Astropy on pypy ? #7768

cdeil opened this issue Aug 24, 2018 · 13 comments

Comments

@cdeil
Copy link
Member

cdeil commented Aug 24, 2018

Recently the Python 3, Numpy and C extension support in pypy has apparently improved a lot.
E.g. in https://mail.python.org/pipermail/scipy-dev/2018-August/022999.html it mentions that scipy now works.

So I ran tests from the latest stable Astropy 3.0.4 with pypy.

It doesn't work yet, there's many fails:

284 failed, 9104 passed, 747 skipped, 63 xfailed, 4298 warnings, 52 error in 364.52 seconds

Actually 3 MB of error log output, see here. :-)

I only skimmed it a bit, it seems to be a mix of issues with the use of esoteric Python features, and lots of fails in the Astropy C extensions (e.g. astropy.io.fits and the ascii C reader / writer in astropy.io.ascii).

There's also many unrelated fails, e.g. the improper use of fixtures in Astropy, resulting in many of those:

E       _pytest.deprecated.RemovedInPytest4Warning: Fixture data called directly. Fixtures are not meant to be called directly, are created automatically when test functions request them as parameters. See https://docs.pytest.org/en/latest/fixture.html for more information.

Also , I noticed that pytest doesn't exit, but hangs at the end. Not sure why, or if it's related to this error printed at the start:

/Users/deil/software/anaconda3/envs/iminuit-270/site-packages/py/_error.py:66: OSError

During handling of the above exception, another exception occurred:

self = <CallInfo when='setup' exception: [Too many open files]: listdir('/private/var/folders/t_/_mywtcj146lbk2c99bnxw7z40000gp/T/pytest-of-deil/pytest-540',)>
func = <function call_runtest_hook.<locals>.<lambda> at 0x000000012196c840>, when = 'setup', treat_keyboard_interrupt_as_exception = False

So clearly Astropy on pypy needs work, probably both in pypy and in Astropy. I have no idea how much in each, whether it's a day or months.

@arigo from pypy - If you have time, maybe you could have a look at the error log and see if there's some things that are helpful for you to improve CPython compatibility in pypy?

@arigo
Copy link

arigo commented Aug 24, 2018

The many EMFILE errors are very likely due to too strong reliance on reference counting, as described here: http://pypy.org/compat.html (see everything after "The main difference that is not going to be fixed is that PyPy does not support refcounting semantics.")

@mhvk
Copy link
Contributor

mhvk commented Aug 26, 2018

Are the fixture errors related to #7769? More generally, it would be good to try to separate this out into different modules, so we can try to attach this incrementally; it would be nice for astropy to work with pypy!

@cdeil
Copy link
Member Author

cdeil commented Aug 26, 2018

Are the fixture errors related to #7769?

Yes. And there's other cases where fixtures are called, e.g. in astropy/stats/lombscargle/tests/test_statistics.py. Those should be sorted out first. I had a very quick look earlier today, but things just seemed broken at the moment (#7772).

Then the errors because Astropy doesn't use with statements to reliably close files in several places should be fixed.

And only then does it make sense to try this again I think and look at the remaining errors from pypy.

Unfortunately I don't have time to contribute to Astropy at the moment and send PRs.

@cdeil
Copy link
Member Author

cdeil commented Jul 12, 2019

At the moment pip_pypy3 install astropy fails like this:

astropy/wcs/src/unit_list_proxy.c:199:5: error: use of undeclared identifier 'Py_RETURN_NOTIMPLEMENTED'

I've opened a feature request with PyPy: https://bitbucket.org/pypy/pypy/issues/3043/support-py_return_notimplemented

@cdeil
Copy link
Member Author

cdeil commented Jul 13, 2019

Current status of Astropy with PyPI:

=============================== 1046 failed, 10181 passed, 946 skipped, 141 xfailed, 682 warnings, 73 error in 475.78 seconds ===============================

Test log (apparently too long for a gist, truncated): https://gist.github.com/cdeil/8d6f5aa6e13baa2f5b634b50cafc0205

I looked through the test fails a bit, there's tons of repeats of the same issue, and many fails that would be easy to fix (slightly different exceptions or warnings behaviour on PyPy, tests that specifically test the CPython response), probably overall only 2-3 real core issues that need to be resolved.

The most obvious one is that PyPy doesn't have sys.getrefcount and I think never all because they don't refcount. We call it four times:
https://github.com/astropy/astropy/search?q=getrefcount&unscoped_q=getrefcount

@mhvk or @taldcroft as astropy.table maintainers - could Table. _replace_column_warnings be rewritten to avoid calling sys.getrefcount or not?

@saimn as astropy.io.fits maintainer - could _File. _maybe_close_mmap be rewritten to avoid calling sys.getrefcount or not?

@MSeifert04
Copy link
Contributor

The maybe_close_mmap probably cannot be re-written currently without getrefcount. However we don't have the backwards-compatibility problem on PyPy and could just close the memmap assuming that was the intention after all, however that could result in some slightly weird differences between PyPy and CPython.

@taldcroft
Copy link
Member

@cdeil - I am a bit concerned about supporting Pypy. I'm not sure how much of an additional development and maintenance burden it will be, but we are already way understaffed for CPython support. So it is a little bit of perspective, i.e. if support for PyPy is considered a bonus or it becomes a requirement for passing CI.

I don't see any obvious way to rewrite Table. _replace_column_warnings without getting ref counts, but there might be someone who is more knowledgable than me.

@cdeil
Copy link
Member Author

cdeil commented Jul 13, 2019

@taldcroft - Agreed. I'll close this issue for now. If others want to come back to this and work on PyPy support in the future, feel free to re-open, or open a new issue.

@cdeil cdeil closed this as completed Jul 13, 2019
@mhvk
Copy link
Contributor

mhvk commented Jul 13, 2019

My sense would that this particular table test can just be skipped on pypy.

@astrofrog
Copy link
Member

Looking at this, there are actually very few unique failures. Most tests are failing because sys.getrefcount is not defined for PyPy. It might make sense to ensure that we have a fallback in this case for portability. Aside from this, there are very few actual failures. As a side note, Travis supports pypy so we could easily add a CI job if we wanted this to work. Not something I have time to work on now though, so I'll leave this closed.

@mhvk
Copy link
Contributor

mhvk commented Dec 18, 2019

It would make sense to keep this open, in case someone else with time to spare wants to have a look... So, reopening.

@mhvk mhvk reopened this Dec 18, 2019
@stonebig
Copy link

@cgohlke published a wheel for PyPy 3.7 at https://www.lfd.uci.edu/~gohlke/pythonlibs/#astropy , so it seems doable.

@astrofrog
Copy link
Member

astrofrog commented Jan 29, 2022

The wheel building machinery we use can build pypy wheels (I think) so we could just try and enable it and see how far we get

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants