Skip to content

BUG, TST: fix f2py for PyPy, skip one test for PyPy#15750

Merged
mattip merged 5 commits intonumpy:masterfrom
mattip:pypy-f2py
Mar 18, 2020
Merged

BUG, TST: fix f2py for PyPy, skip one test for PyPy#15750
mattip merged 5 commits intonumpy:masterfrom
mattip:pypy-f2py

Conversation

@mattip
Copy link
Copy Markdown
Member

@mattip mattip commented Mar 13, 2020

Various changes to make -mfull tests pass on PyPy

  • in f2py, callback argument count was off for PyCFunction_Check with an co_argcount attribute. CPython does not have the attribute, PyPy does.

  • skip a test on PyPy that allocates >6GB memory, somehow it is not collected even with a gc.collect

  • remove the slow marker from some of the f2py tests, I don't think they are that slow.

  • skip tests for tp_doc on PyPy

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers: this is used again on https://github.com/numpy/numpy/pull/15750/files#diff-57556f4d2e5b2a68017772da1827d14cR1100, although I don't really understand what's going on there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that link seems to point here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub bug. It points to line 1100, but only if it's already expanded

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intention is to reduce the argument count by 1 for methods, but rather than check "isinstance(method)" the check is "not a function". Since CFunctions (builtin functions) are not functions, but also do not have co_argcount on CPython, it works for CPython but fails for PyPy.

Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've managed to work out what this create_cb_arglist function even does, can you add a comment on line 1034?

In the long run, it seems much of this could be resolved by just calling inspect.signature, either directly or within numpy.core._internal.py.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a comment to the best of my understanding.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have an understanding of what any of the arguments mean, that would be great in a comment too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the code. It seems to have grown organically as a set of patches over the years, each time fixing one thing with no comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, let's leave this comment as is.

@mattip mattip requested a review from eric-wieser March 16, 2020 08:29
Comment on lines +572 to +573
@pytest.mark.skipif(IS_PYPY,
reason="GC problems after test, even after multiple gc.collect calls")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this worth filing as an issue either in the pypy tracker or the numpy one, and then including the issue id in this reason?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linked to gh-15775

@mattip mattip merged commit 965b41d into numpy:master Mar 18, 2020
@mattip mattip deleted the pypy-f2py branch November 2, 2020 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants