Skip to content
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

CIBW_TEST_SKIP or CIBW_TEST_SELECT #529

Closed
henryiii opened this issue Jan 10, 2021 · 14 comments
Closed

CIBW_TEST_SKIP or CIBW_TEST_SELECT #529

henryiii opened this issue Jan 10, 2021 · 14 comments

Comments

@henryiii
Copy link
Contributor

henryiii commented Jan 10, 2021

This is a proposal for something like CIBW_TEST_SKIP, using build selectors to either eliminate or pick items to run on. After #527, maybe it should be a "positive" selector, since it's now easy to write both positive and negative selectors in one expression, and "CIBW_SKIP" might fade away. Not sure what to call it, CIBW_TEST_BUILD sounds a little odd, but is somewhat obvious that it's the same syntax.


As it is now, I often have to turn off testing on some jobs due building more wheels than upstream requirements (NumPy, generally). It's not that big of an issue to ship with wheel like NumPy missing further up the chain; many users get NumPy from other locations (package mangers, conda, etc). But it's already irritating that I have to turn off all testing when NumPy does support 1 or 2 in the set (PyPy 3.6, or some specific ARM one). I could split the build even further, so that I can have "testable" vs. "not testable". I almost see the use case for CIBW_TEST_SKIP=pp37* ...`. :) But if we do dual-testing on Universal2, that would likely force users to skip all testing if something was not supported, instead of testing half. In fact, if we got ARM runners tomorrow, we might only be able to test on the x86_64 part for some packages! Maybe by the time we get ARM runners, this won't be so common, but some major packages are rather slow to add new wheels. (NumPy. NumPy. NumPy.)

Originally posted by @henryiii in #484 (comment)

@joerick
Copy link
Contributor

joerick commented Jan 10, 2021

As noted in #484, this seems like a sensible route to me. Might also be helpful for people building under emulation where the tests are mega slow.

Perhaps worth debating how universal2 tests might fit into this, as maybe we could kill two birds with one stone. Could we have some identifiers like cp39-macosx_universal2[arm64] cp39-macosx_universal2[x86_64] that identified different archs within a single wheel... that might relieve the need for the previously mentioned CIBW_TESTS_ARCHS.

After #527, maybe it should be a "positive" selector, since it's now easy to write both positive and negative selectors in one expression, and "CIBW_SKIP" might fade away.

I prefer skip, personally, as it's more natural to start with all the builds running tests, then remove them (default: `` 'skip nothing'). (I also don't think CIBW_SKIP is going anywhere, for similar reasons.)

@YannickJadoul
Copy link
Member

I prefer skip, personally, as it's more natural to start with all the builds running tests, then remove them (default: `` 'skip nothing'). (I also don't think CIBW_SKIP is going anywhere, for similar reasons.)

If we can extend the braces syntax, that should also help making things concise enough?

@henryiii
Copy link
Contributor Author

henryiii commented Jan 10, 2021

The positive expression with ! lets you write this, so that CIBW_TEST_SELECT: !pp* would deactivate all PyPy archs, versus forcing negative selections CIBW_TEST_SKIP: pp* , which would then mean you couldn't do something like CIBW_TEST_SELECT: *arm64 to request only arm64 wheels be tested. But I would assume the "norm" would be skipping, so up to you. I'm fond of the idea of focusing on a single, consistent build selector instead of combining two since we have a ! syntax to use now, but it's only a preference, and might not be shared by users who don't look at the doc (update)s.

We might have to be a bit careful with adding an extra bit to the testing selector, so as not to be too surprising with CIBW_TEST_SKIP: *universal2 not actually removing anything. Also, we shouldn't use square brackets, as they interfere with fnmatch.

Maybe we could take advantage of the fact universal2 is made up of arm64 and x86_x64. So if macosx_arm64 is allowed, the ARM64 part of the universal2 wheel is tested. If macosx_x86_64 is allowed, then the x86_64 part is tested. So if you only wanted to test "native" builds on arm, you could write:

CIBW_TEST_SKIP: "*x86_64"
# Or, we we used a "positive" expression, either of these would work:
CIBW_TEST_SELECT: "!*x86_64"
CIBW_TEST_SELECT: "*arm64"

Only downside is if you build a Universal2 wheel and a dedicated wheel, and you wanted to test the dedicated wheel but not the universal2 wheel part that matches the dedicated wheel, you couldn't do that. But that sounds totally reasonable to me, either you support testing an arch or you don't (in a job).

@henryiii henryiii changed the title CIBW_TEST_SKIP or similar CIBW_TEST_SKIP or CIBW_TEST_SELECT Jan 10, 2021
@henryiii
Copy link
Contributor Author

henryiii commented Jan 10, 2021

Reminder, here are the docs for the wcmatch.fnmatch syntax we can use: https://facelessuser.github.io/wcmatch/fnmatch/#negateall

That's exactly what we have now but in one line: a positive pattern selects, negative pattern subtracts, only negative patterns subtract from all.

There are no ordering issues, order doesn't matter.

Since we will be showing examples with no previous legacy behavior to worry about there, we could easily show examples like CIBW_TEST_SELECT: !*x86_64 and might even help people learn that it's now possible with CIBW_BUILD, too. I'm open, though.

@YannickJadoul
Copy link
Member

I would indeed expect skipping tests to be the exception (and maybe we should try encouraging that?), and in most/almost all cases CIBW_SKIP_TEST to be a list containing those few exceptions. But yes, with ! in front, that would work just as well (it's mainly switching perspective).

I think I'm lightly for CIBW_TEST_SKIP, as those are two words we already use in options (CIBW_TEST_COMMAND and CIBW_SKIP), so it would almost be guessable (though the analogy won't fully hold, because CIBW_TEST_BUILD is not proposed and is a horrible name, using "build" for running tests).

@henryiii
Copy link
Contributor Author

henryiii commented Jan 11, 2021

I can imagine a couple of use cases for a positive expression:

# Test only one version of Python, since the test suite
# is slow and if one works, they all should
CIBW_TEST_SELECT: "cp39*"

# Test only 'normal' architectures on Linux, ignore emulated ones
CIBW_TEST_SELECT: "*_x86_64 *_i686"

These are cooked up, not necessarily real or best practice.

You could also argue it might be more elegant to look up the list of identifiers NumPy supports (you could even share this list), rather than just finding the intersection between what you support and what NumPy does not.

@YannickJadoul
Copy link
Member

Hmmm, true. I hadn't considered these, but "Only Python 3.9, because tests are expensive" could make sense, yes (which doesn't mean that CIBW_TEST_SKIP might not be more useful/intuitive, most of the time, though; but I don't have a use case myself, so hard to say for sure). Anyway, back to @joerick ;-)

(I don't really like the CIBW_TEST_SELECT name, though; the inconsistency between CIBW_BUILD and CIBW_TEST_SELECT annoys me more than it should.)

@henryiii
Copy link
Contributor Author

henryiii commented Jan 11, 2021

We could use CIBW_SELECT with ! syntax enabled, or CIBW_BUILD + CIBW_SKIP with ! disabled (and it would be an error to specify both BUILD and SELECT). That's definitely on the path of moving away from SKIP, though. That was the original proposal of #516, actually; though it included PLATFORM + ARCH; I would argue we should never include PLATFORM, since "normal" (ci) usage never should set it, and it's there more as a protection against locally modifying your Win/macOS system as well as running in docker on macOS/Windows, which CI should never do. And then ARCH and MIN_VERSION enable "special" behaviors that are not ideally forced into BUILD/SKIP/SELECT.

You could keep SKIP around as an extra level of filter, but still go this path, I suppose. It could be handy to just globally disable pp*, etc.

@joerick
Copy link
Contributor

joerick commented Jan 15, 2021

CIBW_TEST_SKIP still seems to cover the main use-cases, for me, and is going to be more obvious what it does when scanning the option names.

An idea for the universal2 archs - we could call the parts of a universal2 wheel cp39-macosx_universal2/x86_64 and cp39-macosx_universal2/arm64, with a selector of cp39-macosx_universal2 matching (skipping) them both. (Analogy to folder/files.) Looks a bit weird but won't conflict with any of the glob syntax!

@henryiii
Copy link
Contributor Author

I think it would be simpler to not introduce new selectors. We list all selectors explicitly, this would require a special listing just for TEST_SKIP.

Oh, so it's tricking this as a folder/file. 🤯 Have to think about it, maybe that would work?

@henryiii
Copy link
Contributor Author

Oops, probably should wait until the universal part has been added.

@joerick
Copy link
Contributor

joerick commented Jan 26, 2021

For the universal2 parts, I've settled on a colon, e.g. cp39-macosx_universal2:arm64, in the end. It just looked a little less strange for whatever reason!

@YannickJadoul
Copy link
Member

That looks quite good, indeed!

@henryiii
Copy link
Contributor Author

henryiii commented Feb 1, 2021

We've added #484 , so I think this is done!

@henryiii henryiii closed this as completed Feb 1, 2021
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

No branches or pull requests

3 participants