hypothesis icon indicating copy to clipboard operation
hypothesis copied to clipboard

`extra.array_api` versioning + complex support

Open honno opened this issue 2 years ago • 3 comments

WIP PR to resolve https://github.com/HypothesisWorks/hypothesis/issues/3422. That means 1) complex support 2) versioning namespaces. Not particularly looking for a thorough review just yet, but would of-course appreciate feedback and ideas.

Prior brain dump

Complex support

Per point 1, supporting complex arrays has consisted of my initial proposal

Versioned strategy namespaces

Per point 2, make_strategies_namespace() now takes an api_version kw-only argument per @Zac-HD's suggestion. The argument

  • defaults to ..., which indeed tries known versions (descending) and returns the first that works.
  • can be a released versions string (i.e. just "2021.12" for now), which forcibly returns a strategies namespace for the given version.
  • can be "draft", which lets us access a namespace with no guarantees (documented as such). The expectation is that the namespace would follow the draft spec. This is useful to implement and internally test features before spec releases (e.g. complex numbers)... and helps me out with array-api-tests :sweat_smile:
  • Anything else, including None, raises a hopefully useful error. Might need to mull it over, and write additional context if we detect None.

Currently in a 2021.12 namespace, numeric_dtypes() doesn't generate complex numbers, and complex_dtypes() doesn't exist. draft conversely has the complex support as outlined a bit above.

Internal test suite

The big TODO is updating the test suite. As of writing, currently I've introduced xp-agnostic tests for make_strategies_namespace(), and tests for new complex behaviour.

Parametrizing versioned namespaces?

Per tests/array_api/conftest.py (see #3085 and #3275), we need to consider how versioning interacts with parametrization of array modules. See its README.

I think ideally

  • when specifying an array module, default to just testing with the strategies namespace we get from make_strategies_namespace(...).
  • add support for specifying a specific version for said array module(s).
  • by default, test our mock_xp on "draft". Currently we test with NumPy-proper if available, but this is limiting in testing newer features (e.g. complex-numbers). Or maybe test both mock_xp on "draft" and numpy on inferred latest version.

Version-specific testing

Mark tests which are only for YYYY.MM+ versions?

For the complex-related tests which get parametrize via contest.py, my idea was/is to introduce a marker xp_min_version(api_version), e.g.

# test_scalar_dtypes.py
@pytest.mark.xp_min_version("draft")
def test_can_generate_complex_dtypes(xp, xps):
    ...

# test_arrays.py
dtype_name_params = ["bool",] + list(REAL_NAMES) + [
    pytest.param(n, marks=pytest.mark.xp_min_draft("draft")]
]

@pytest.mark.parametrize("dtype_name", dtype_name_params)
def test_draw_arrays_from_dtype(...):
    ...

@pytest.mark.parametrize("dtype_name", dtype_name_params)
def test_draw_arrays_from_scalar_names(...):
    ...

I thought I could get the xps argument from a pytest item in the collection_modifyitems hook, which would hold a newly introduced api_version attribute. For items with xp_min_version() markers, that xps.api_version value could of been checked against the min version (e.g. from <marker>.args[0]), where when "less-than" a skip marker would be added to the item... my first go didn't work out heh, so will sleep on it.

For some tests, change test behaviour on runtime?

Also additionally xps.api_version could maybe change behaviour of tests like

# test_scalar_dtypes.py
def test_can_generate_numeric_dtypes(xp, xps):
    if api_version_gt(xps.api_version, "2021.12"):
        dtype_names = <includes complex dtypes>
    else:
        dtype_names = <doesn't include complex dtypes>
    numeric_dtypes = [getattr(xp, n) for n in dtype_names]
    assert_all_examples(xps.numeric_dtypes(), lambda dtype: dtype in numeric_dtypes)

Might want to create separate tests, will mull over.

TODO:

  • [ ] Review the diff and see if there's anything I could clean up easily.
  • [ ] RELEASE.rst and other things CI picks on
  • [x] Update the test suite.
  • [ ] Update/add some API docs.
  • [ ] Add test docstrings (namely to force me to review them heh).

honno avatar Sep 13 '22 18:09 honno

Thanks for the suggestions! Other than an updated TODO list, think I'm now happy with the API and general architecture things, but something to sleep on.

I updated the tests/array_api/ README which should explain my current solution to namespace version paramtrization. Basically by default the latest supported version is used, and a new env lets you specify a specific version. I didn't introduce an "all" option as ugh I need to think about it :upside_down_face: I guess a future problem is you can end up not testing with all released versions by default, just for now on CI you should have draft covered (by the mock) and 2021.12 covered by numpy.array_api.

honno avatar Sep 19 '22 12:09 honno

Having gone through it again, I don't think there are any major changes; just a little polish and I'll be happy to merge.

I updated the tests/array_api/ README which should explain my current solution to namespace version paramtrization. Basically by default the latest supported version is used, and a new env lets you specify a specific version. I didn't introduce an "all" option as ugh I need to think about it 🙃 I guess a future problem is you can end up not testing with all released versions by default, just for now on CI you should have draft covered (by the mock) and 2021.12 covered by numpy.array_api.

I'm fine with leaving a note (see comment above), and otherwise deciding that we'll deal with this later. More, smaller PRs is good!

Zac-HD avatar Sep 20 '22 06:09 Zac-HD

Linting, type checking, and release notes - then I think we're ready to merge!

(though FYI I'm away thurs-mon, so might get to this late next week)

Zac-HD avatar Sep 21 '22 16:09 Zac-HD

So in regards to xp.__array_api_version__ https://github.com/HypothesisWorks/hypothesis/pull/3456#discussion_r976412249, the PR now uses that for inferrence, and ditches the funky x.__array_namespace__() method (save here in case). Tests have been updated accordingly.

For the test/array_api/ conf, I now just default to testing with the mock, as it seems rather bothersome to raise a warning or whatever for default runs of the test suite (for CI but also developer experience). README.md updated. Also numpy.array_api might not exist/be updated soon enough? https://github.com/numpy/numpy/issues/22252

Windows tests seem to be failing because the custom markers registered in tests/array_api/conftest.py are well not being registered heh. Will explore next week.

honno avatar Sep 23 '22 16:09 honno

Thanks for the great review comments, should be all addressed now.

I think after this round we're probably just waiting for a decision on complex-dtype introspection?

Technically this PR is compliant to the current draft spec, so it seems waiting is a question of do we want an inevitable future PR.

I'm 99% sure complex support for xp.finfo() (https://github.com/data-apis/array-api/pull/484) will get merged, given we agreed upon it in a recent consortium meeting. Spec triage is always variable and our resident reviewer is currently out, so it might not get merged say this week... we could shoot the gun here even?

Also I noticed nps.from_dtype() currently doesn't support min/max magnitude arguments—once we resolve complex-dtype introspection, it's okay to support those args for xps.from_dtype() at least I assume?

honno avatar Sep 27 '22 10:09 honno

I noticed nps.from_dtype() currently doesn't support min/max magnitude arguments—once we resolve complex-dtype introspection, it's okay to support those args for xps.from_dtype() at least I assume?

Absolutely, also happy to support in xps too when that makes sense - and since it's a logically separate feature, please post it as a separate PR!

Zac-HD avatar Sep 28 '22 07:09 Zac-HD

🚢🚢:shipit:

Zac-HD avatar Sep 29 '22 19:09 Zac-HD

Awesome, thanks again Zac!

honno avatar Sep 29 '22 21:09 honno