Skip to content

fix: Import backports-datetime-fromisoformat only if needed, to fix PyPy 3.7 support #415

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

Closed
wants to merge 2 commits into from

Conversation

jpmckinney
Copy link
Contributor

@jpmckinney jpmckinney commented Dec 19, 2022

Replaces #411

I'm not sure why the pypy tests fail. It's maybe a package in the test environment.

Anyhow, the important bit is that we don't see this error:

  File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/flattentool/input.py", line 23, in <module>
    from flattentool.ODSReader import ODSReader
  File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/flattentool/ODSReader.py", line 21, in <module>
    import backports.datetime_fromisoformat
  File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/backports/datetime_fromisoformat/__init__.py", line 4, in <module>
    from backports._datetime_fromisoformat import date_fromisoformat, datetime_fromisoformat, time_fromisoformat, FixedOffset
ImportError: /opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/backports/_datetime_fromisoformat.pypy37-pp73-x86_64-linux-gnu.so: undefined symbol: _PyUnicode_Copy

I can undo the change to test.yml.

@ghost
Copy link

ghost commented Jan 11, 2023

Can we just check, is this the only change you need for your pypy use case or would there be more to come?

If this is it, we'd be happy to merge it and leave the pypy tests as expected failures for now until we can properly review pypy. We'll just add something to document that in the code base.

@jpmckinney
Copy link
Contributor Author

This is it!

ghost pushed a commit that referenced this pull request Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

I can undo the change to test.yml.

Having looked more into pypy and thinking about it, I think we'd prefer not to have in Github Actions actually.

Closing for #417 which also releases this

@ghost ghost closed this Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

0.20.1 released

@jpmckinney jpmckinney deleted the pypy-support branch January 11, 2023 20:37
@jpmckinney
Copy link
Contributor Author

jpmckinney commented Jan 11, 2023

Curious what your thoughts were on PyPy? David's testing in #377 had about 2x speed improvement.

@ghost
Copy link

ghost commented Jan 12, 2023

  • A combination of noting Pypy is only for older versions (no criticism intended - I understand it's a massive project!), the performance boosts delivered in cPython 3.11 and the performance boosts being talked about for cPython 3.12. This leads me towards thinking our first performance work should be making sure our libraries work great on newer versions of Python before looking at PyPy and the additional complexities that brings. (As we use Docker for deployment more we are able to use later pythons and not be reliant on what version of Python Ubuntu LTS ships, as in the past).
  • Not wanting to suggest to people we are working towards full Pypy compatibility if we are not.
  • Rethinking having failing tests - in theory they are still useful because people can check them by hand and celebrate progress and avoid regressions. However we already have these in some places, and I was thinking about that and realising that in practice what happens is people ignore them and just get annoyed by the alert emails.

This isn't a firm co-op policy, just my quick thoughts to try and make sure this bit of work was progressed without being blocked on us going away and making a firm co-op policy - happy to have a general discussion about Pypy for this and other libraries.

@jpmckinney
Copy link
Contributor Author

jpmckinney commented Jan 12, 2023

Thank you for sharing!

As I understand, PyPy is expected to always be considerably faster than cpython, as it uses a JIT. It seems to usually be two minor versions behind, but it's been a while since I've needed a bleeding-edge language feature.

FWIW, in my experience, PyPy compatibility has not required any changes most of the time.

And, of course, backports-datetime-fromisoformat, but it's only needed for < Python 3.7.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant