Skip to content

Fix test_solve generating arrays larger than our max array size #242

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

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

honno
Copy link
Member

@honno honno commented Mar 11, 2024

Resolves #238—the end side of the second test_solve array shape needed to be ceiled relative to the rest of the shape size, otherwise the array size could exceed our desired maximum!

@asmeurer
Copy link
Member

With this PR, I get the following error https://gist.github.com/asmeurer/99eabdef967432ab2ab36ef36a34d6f4 from running

ARRAY_API_TESTS_MODULE=numpy.array_api pytest array_api_tests/test_linalg.py -k solve --max-examples=1000

with hypothesis 6.99.2.

@asmeurer
Copy link
Member

The change here seems correct, though maybe we should get it fully working before merging.

If the source of this error is passing too large of a shape to arrays perhaps hypothesis should have a separate check for this in arrays, or else intercept the error there and translate it to a more meaningful one.

@Zac-HD
Copy link

Zac-HD commented Mar 11, 2024

If the source of this error is passing too large of a shape to arrays perhaps hypothesis should have a separate check for this in arrays, or else intercept the error there and translate it to a more meaningful one.

So far as I can tell, this strategy does not go through arrays().

@asmeurer
Copy link
Member

There's this line https://github.com/data-apis/array-api-tests/pull/242/files#diff-6056c0b3af9cd3ba66387432a17f5f36bbd54220419656441a8b01bcdc4df44bR619.

Also, I'll need to check again tomorrow, but from what I recall from a week ago when I ran this through a debugger there was some line in hypothesis in the arrays code that looked like it was what was generating the long list (but I also found it very difficult to tell what was going on inside of the hypothesis stack in the debugger).

@honno
Copy link
Member Author

honno commented Mar 12, 2024

If the source of this error is passing too large of a shape to arrays perhaps hypothesis should have a separate check for this in arrays, or else intercept the error there and translate it to a more meaningful one.

So far as I can tell, this strategy does not go through arrays().

Looks like our hh.invertible_matrices() strategy does use arrays() and like before there seemed to be a case where the final array shape could exceed our max size, so pushed another fix if you could take a gander @asmeurer.

ARRAY_API_TESTS_MODULE=numpy.array_api pytest array_api_tests/test_linalg.py -k solve --max-examples=1000

On my end I get a failed deadline health check for this, both before and after my changes 🤔 When ignoring the deadline (from hypothesis import settings; @settings(deadline=None)) the test passes locally both before and after my changes too.

@asmeurer
Copy link
Member

Found the line in hypothesis I was thinking about. It's actually in the traceback in the gist I posted

    |   File "/Users/aaronmeurer/miniconda3/envs/array-apis/lib/python3.11/site-packages/hypothesis/extra/array_api.py", line 369, in do_draw
    |     elems = data.draw(
    |             ^^^^^^^^^^

If I am reading this correctly, arrays does indeed use lists https://github.com/HypothesisWorks/hypothesis/blob/5eeb51ad38a44fefdc4b82dc0062487968ee089a/hypothesis-python/src/hypothesis/extra/array_api.py#L370

@asmeurer
Copy link
Member

On my end I get a failed deadline health check for this, both before and after my changes 🤔 When ignoring the deadline (from hypothesis import settings; @settings(deadline=None)) the test passes locally both before and after my changes too.

I don't know. Maybe it requires a lot of examples. Indeed if invertible_matrices alone can cause this I've never seen it in other tests that use it. I just ran a whole bunch of examples for test_solve and I don't see this any more in this branch.

By the way, in the test suite you can disable the deadline with --disable-deadline.

@honno
Copy link
Member Author

honno commented Mar 13, 2024

I just ran a whole bunch of examples for test_solve and I don't see this any more in this branch.

So in any case, if you're not getting a failed health check now then you/I can merge this? And I'll mull over the other points 😅

@asmeurer
Copy link
Member

I did get some deadline errors, but I ignored them.

@asmeurer asmeurer merged commit 5811393 into data-apis:master Mar 13, 2024
@honno honno deleted the solve-buffer-size-fix branch April 15, 2024 16:34
@asmeurer asmeurer mentioned this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants