GeezerVison

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

All commit authors signed the Contributor License Agreement.

@terryjreedy How does this look?

Thanks @hugovk @Zac-HD @erlend-aasland @terryjreedy and everyone else who helped with this!

Looking forward to making more use of this in the future!

Looks like this is causing some buildbot failures on main: https://buildbot.python.org/all/#/builders/464/builds/4434/steps/7/logs/stdio

Anecdotal feedback: On the current CI run on #103764, the Hypothesis tests are the slowest part of the CI run.

Seems like it's running the full test suite, maybe it should run only test_zoneinfo or whatever tests actually use Hypothesis?

Anecdotal feedback: On the current CI run on #103764, the Hypothesis tests are the slowest part of the CI run.

The slowest part of the CI run by a long way: they took 46 minutes to complete on #104421, whereas no other job took longer than 26 minutes.

I agree with @JelleZijlstra -- changing the workflow file so that this job only runs on PRs that affect zoneinfo seems like a good idea?

Looks like this is causing some buildbot failures on main: https://buildbot.python.org/all/#/builders/464/builds/4434/steps/7/logs/stdio

I think the issue here is that the new _hypothesis_stubs directory doesn't get picked up by make install. I see how to fix that, will send a PR.

Anecdotal feedback: On the current CI run on #103764, the Hypothesis tests are the slowest part of the CI run.

Seems like it's running the full test suite, maybe it should run only test_zoneinfo or whatever tests actually use Hypothesis?

The problem with this is that there's no easy way for unittest to automatically identify which tests are using hypothesis, and when new tests using hypothesis are added, they'd have to manually be added to this job, which everyone will forget to do. The compromise I'm currently using is to filter out the slowest non-hypothesis tests until we have some ability to do more granular filters.

Anecdotal feedback: On the current CI run on #103764, the Hypothesis tests are the slowest part of the CI run.
Seems like it's running the full test suite, maybe it should run only test_zoneinfo or whatever tests actually use Hypothesis?

The problem with this is that there's no easy way for unittest to automatically identify which tests are using hypothesis, and when new tests using hypothesis are added, they'd have to manually be added to this job, which everyone will forget to do. The compromise I'm currently using is to filter out the slowest non-hypothesis tests until we have some ability to do more granular filters.

I feel like adding new hypothesis tests will be rare enough that doing it manually isn't too bad.

The problem with this is that there's no easy way for unittest to automatically identify which tests are using hypothesis, and when new tests using hypothesis are added, they'd have to manually be added to this job, which everyone will forget to do.

I agree that that doesn't sound ideal, but honestly it doesn't sound as bad (to me) as having to wait 45 minutes for CI to complete on every PR.

I feel like adding new hypothesis tests will be rare enough that doing it manually isn't too bad.

I sure hope not! hypothesis is extremely useful, particularly for the kind of things we do here. The only reason I haven't added them to almost every change I've added to datetime is because I couldn't. We've got a whole bunch more to be imported from here as well.

I agree that that doesn't sound ideal, but honestly it doesn't sound as bad (to me) as having to wait 45 minutes for CI to complete on every PR.

I don't really understand why this is so slow TBH. It's running the same test suite as the Ubuntu job, plus the parts of test_zoneinfo that run property tests. Is test_zoneinfo actually going slow, or does the test suite just run really slowly in a virtual environment for some reason?

I think we should solve the root problems here. unittest or python -m test should grow the ability to have "run only these tests" functionality rather than just "don't run these tests", and we should figure out why it's so slow to run these tests in CI.

I feel like adding new hypothesis tests will be rare enough that doing it manually isn't too bad.

I sure hope not! hypothesis is extremely useful, particularly for the kind of things we do here. The only reason I haven't added them to almost every change I've added to datetime is because I couldn't. We've got a whole bunch more to be imported from here as well.

Agree that we'll hopefully add similar tests to more files! Still, pull requests that add hypothesis tests to a new file won't be terribly common, I expect.

I agree that that doesn't sound ideal, but honestly it doesn't sound as bad (to me) as having to wait 45 minutes for CI to complete on every PR.

I don't really understand why this is so slow TBH. It's running the same test suite as the Ubuntu job, plus the parts of test_zoneinfo that run property tests. Is test_zoneinfo actually going slow, or does the test suite just run really slowly in a virtual environment for some reason?

I don't think it's just test_zoneinfo; if I'm reading the logs in https://github.com/python/cpython/actions/runs/4960065291/jobs/8875040006?pr=103764 right, zoneinfo itself completed in 10 s or so. Actually I think the issue may be that the tests are all run serially, while the normal CI run uses -j4. On the normal runs, all the tests ran in 13 min on my PR, and the hypothesis run took 41 min.

I think we should solve the root problems here. unittest or python -m test should grow the ability to have "run only these tests" functionality rather than just "don't run these tests", and we should figure out why it's so slow to run these tests in CI.

FYI I just got a test_zoneinfo failure from the new job in #104421 (which doesn't touch anything to do with zoneinfo)

FYI I just got a test_zoneinfo failure from the new job in #104421 (which doesn't touch anything to do with zoneinfo)

Yeah I think we should tweak the hypothesis settings to make it a bit less flaky.

@Zac-HD Any interest in helping with that?

(To update, for anybody reading this thread in the future: the extremely long time taken by the test was fixed by #104427. It now only takes 13 minutes in CI 🎉)

I think we should solve the root problems here. unittest or python -m test should grow the ability to have "run only these tests" functionality rather than just "don't run these tests"

FWIW you can detect this by checking for a .hypothesis attribute on the test function or method; I guess in unittest you'd create a custom TestLoader subclass which knows how to check this attribute?

I think we should tweak the hypothesis settings to make it a bit less flaky. @Zac-HD Any interest in helping with that?

Happy to, though I think that the -j 4 and stubs-install fixes have resolved this? This failure Alex mentions is a segfault in a threading test, which I don't think is fixable by configuring Hypothesis.

This failure Alex mentions is a segfault in a threading test, which I don't think is fixable by configuring Hypothesis.

No, that's a separate failure on a later commit in that PR. The test failure on the previous commit in that PR is the relevant one that I was talking about.

@Zac-HD here's a direct link to the run where test_zoneinfo failed on #104421: https://github.com/python/cpython/actions/runs/4960409839/jobs/8875856040

Oh, I see, sorry. Expect a PR to set deadline=None globally tomorrow 🙂

One interesting thing -- and this definitely isn't due to hypothesis, but might be due to the specific way that the test suite is being invoked in CI -- is that test_threading and test__xxsubinterpreters seem to be crashing constantly in the new "Hypothesis Tests on Ubuntu" job in CI (see https://github.com/python/cpython/actions/runs/4966613914/jobs/8888147069?pr=104421 for another example). These test failures had been observed before (see #104341), so they definitely aren't caused by the addition of the new CI job. But on other jobs in CI, the crashes had been sporadic/intermittent; on this new job, they seem to be crashing all the time.

I don't know if that helps @ericsnowcurrently debug the root cause of the crashes at all!

FYI, gh-105109 should resolve the failures.