Skip to content

Skip extension tests if the respective extension is not found or is passed via --disable-extensions #29

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 15 commits into from
Oct 22, 2021

Conversation

honno
Copy link
Member

@honno honno commented Oct 14, 2021

Should resolve #25. My current idea is to add a pytest options --extensions, where the user can specify any extensions they want to test alongside the normal tests (i.e. by default, no extensions are tested).

This works by catching a custom marker xp_extension(<ext>) in pytest's collection hook—if the user has has passed the respective <ext> then the test is run, otherwise it's skipped. For parameters it can just be passed in the marks kwarg e.g. pytest.param(..., marks=xp_extension("linalg")), and for module level it can be defined in a file's top-level variable pytestmark e.g.:

pytestmark = [pytest.mark.xp_extension("linalg")]

I think I understand how test_signatures.py works now, so I tomorrow I should have a ready solution to easily opt-in the linalg tests there and in test_linalg.py.

@asmeurer
Copy link
Member

Is it possible to by default test an extension if it appears to be installed (if _array_module.extension is not _UndefinedStub)?

@honno honno changed the title Make extensions opt-in via an --extensions ... option Skip extension tests if the respective extension is not found or is passed via --disable-extensions Oct 15, 2021
@honno
Copy link
Member Author

honno commented Oct 15, 2021

Is it possible to by default test an extension if it appears to be installed (if _array_module.extension is not _UndefinedStub)?

Sure thing. So now this PR skips extension tests if they're not found, otherwise testing them by default. If the user wants to purposely skip extension tests, they can use something like --disable-extension linalg.

@honno
Copy link
Member Author

honno commented Oct 15, 2021

For future reference—the current NumPy workflow conflicts with a contest.py that specifies a pytest_collection_modifyitems hook. I had actually moved conftest.py originally to inside of array_api_tests/ because I wanted to import some things, but now have kept the original conftest.py in the repo's root and left the new config at array_api_tests/conftest.py. Note that pytest allows and seems to encourage dir-level conftest.py files for more granular settings anywho (which is "merged" with the parent configs too).

@honno honno marked this pull request as ready for review October 18, 2021 09:12
@honno
Copy link
Member Author

honno commented Oct 18, 2021

I forgot to undraft this last week heh. Happy for a review.

@asmeurer
Copy link
Member

Looks like this has conflicts now.

@honno
Copy link
Member Author

honno commented Oct 19, 2021

@asmeurer fixed, fortunately just a rebase did the trick.

@asmeurer
Copy link
Member

The GitHub actions workflow should be appending to conftest (it uses >>). So I think it ought to work.

If you have any suggestions for a better way to do it, let me know. I didn't get any good answers at https://stackoverflow.com/questions/68764967/xfail-pytest-tests-from-the-command-line. Maybe we should open a feature request for pytest to make something like this easier. Ideally you could just do pytest --xfail-tests ....

@asmeurer
Copy link
Member

Let's document this in the README.

@asmeurer
Copy link
Member

I keep getting errors like this

xp = <module 'array_api_tests._array_module' from '/Users/aaronmeurer/Documents/array-api-tests/array_api_tests/_array_module.py'>, dtype = dtype('bool')
shape = <function accept.<locals>.matrix_shapes at 0x113aef8b0>

    def _arrays(
        xp: Any,
        dtype: Union[DataType, str, st.SearchStrategy[DataType], st.SearchStrategy[str]],
        shape: Union[int, Shape, st.SearchStrategy[Shape]],
        *,
        elements: Optional[st.SearchStrategy] = None,
        fill: Optional[st.SearchStrategy[Any]] = None,
        unique: bool = False,
    ) -> st.SearchStrategy:
        """Returns a strategy for :xp-ref:`arrays <array_object.html>`.

        * ``dtype`` may be a :xp-ref:`valid dtype <data_types.html>` object or name,
          or a strategy that generates such values.
        * ``shape`` may be an integer >= 0, a tuple of such integers, or a strategy
          that generates such values.
        * ``elements`` is a strategy for values to put in the array. If ``None``
          then a suitable value will be inferred based on the dtype, which may give
          any legal value (including e.g. NaN for floats). If a mapping, it will be
          passed as ``**kwargs`` to :func:`from_dtype()` when inferring based on the dtype.
        * ``fill`` is a strategy that may be used to generate a single background
          value for the array. If ``None``, a suitable default will be inferred
          based on the other arguments. If set to
          :func:`~hypothesis.strategies.nothing` then filling behaviour will be
          disabled entirely and every element will be generated independently.
        * ``unique`` specifies if the elements of the array should all be distinct
          from one another; if fill is also set, the only valid values for fill to
          return are NaN values.  Note that Hypothesis always allows multiple NaN
          values, even though `xp.unique() might only return a single NaN.
          <https://data-apis.org/array-api/latest/API_specification/set_functions.html#objects-in-api>`__

        Arrays of specified ``dtype`` and ``shape`` are generated for example
        like this:

        .. code-block:: pycon

          >>> from numpy import array_api as xp
          >>> xps.arrays(xp, xp.int8, (2, 3)).example()
          Array([[-8,  6,  3],
                 [-6,  4,  6]], dtype=int8)

        Specifying element boundaries by a :obj:`python:dict` of the kwargs to pass
        to :func:`from_dtype` will ensure ``dtype`` bounds will be respected.

        .. code-block:: pycon

          >>> xps.arrays(xp, xp.int8, 3, elements={"min_value": 10}).example()
          Array([125, 13, 79], dtype=int8)

        Refer to :doc:`What you can generate and how <data>` for passing
        your own elements strategy.

        .. code-block:: pycon

          >>> xps.arrays(xp, xp.float32, 3, elements=floats(0, 1, width=32)).example()
          Array([ 0.88974794,  0.77387938,  0.1977879 ], dtype=float32)

        Array values are generated in two parts:

        1. A single value is drawn from the fill strategy and is used to create a
           filled array.
        2. Some subset of the coordinates of the array are populated with a value
           drawn from the elements strategy (or its inferred form).

        You can set ``fill`` to :func:`~hypothesis.strategies.nothing` if you want
        to disable this behaviour and draw a value for every element.

        By default ``arrays`` will attempt to infer the correct fill behaviour: if
        ``unique`` is also ``True``, no filling will occur. Otherwise, if it looks
        safe to reuse the values of elements across multiple coordinates (this will
        be the case for any inferred strategy, and for most of the builtins, but is
        not the case for mutable values or strategies built with flatmap, map,
        composite, etc.) then it will use the elements strategy as the fill, else it
        will default to having no fill.

        Having a fill helps Hypothesis craft high quality examples, but its
        main importance is when the array generated is large: Hypothesis is
        primarily designed around testing small examples. If you have arrays with
        hundreds or more elements, having a fill value is essential if you want
        your tests to run in reasonable time.
        """
        check_xp_attributes(
            xp, ["asarray", "zeros", "full", "all", "isnan", "isfinite", "reshape"]
        )

        if isinstance(dtype, st.SearchStrategy):
            return dtype.flatmap(
                lambda d: _arrays(xp, d, shape, elements=elements, fill=fill, unique=unique)
            )
        if isinstance(shape, st.SearchStrategy):
            return shape.flatmap(
                lambda s: _arrays(xp, dtype, s, elements=elements, fill=fill, unique=unique)
            )

        if isinstance(dtype, str):
            dtype = dtype_from_name(xp, dtype)

        if isinstance(shape, int):
            shape = (shape,)
        check_argument(
>           all(isinstance(x, int) and x >= 0 for x in shape),
            f"shape={shape!r}, but all dimensions must be non-negative integers.",
        )
E       TypeError: 'function' object is not iterable

../../anaconda3/envs/array-apis/lib/python3.9/site-packages/hypothesis/extra/array_api.py:482: TypeError

The problem, which always takes me some time to figure out, is that I accidentally did xps.arrays(shape=matrix_shapes) instead of xps.arrays(shape=matrix_shapes()). Might be worth doing some better type checking for shape in hypothesis.

@asmeurer
Copy link
Member

Just pushed a change to master that should make the tests pass here once you merge it.

@honno honno requested a review from asmeurer October 20, 2021 10:11
@honno
Copy link
Member Author

honno commented Oct 20, 2021

Now the user can purposely test an extension via --enable-extension, but note these tests are run by default if they are found. I've updated the README to explain options use.

I see the issue with workflows is tricky. I tried a few things, but I think I like this the most: have a xfails.txt file in the repo root that stores test ids to xfail, to subsequently be read by conftest.py. For workflows this means catting into xfails.txt, as you can see. This means we don't have to touch conftest.py (so I can merge my new child config into the top-level config), and I believe should be a solution which is slightly easier for others to understand.

@asmeurer
Copy link
Member

You might as well also update the XFAILs. Some ones need to be added (I'm not sure why floor_divide and to_device were not already listed), and some can be removed now that numpy/numpy#19937 is merged.

@honno
Copy link
Member Author

honno commented Oct 21, 2021

Updated the README appropriately. Also updated the xfails—the NumPy workflow still fails, but that should be fixed in #31.

@honno honno requested a review from asmeurer October 21, 2021 16:29
@asmeurer asmeurer merged commit 7c53ded into data-apis:master Oct 22, 2021
@honno honno deleted the skip-extensions branch February 8, 2022 10:05
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.

Clean way to optional enable/disable extensions
2 participants