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

test_implied_dirs_performance is flaky #102209

Closed
jaraco opened this issue Feb 24, 2023 · 5 comments · Fixed by jaraco/zipp#94
Closed

test_implied_dirs_performance is flaky #102209

jaraco opened this issue Feb 24, 2023 · 5 comments · Fixed by jaraco/zipp#94
Labels
type-bug An unexpected behavior, bug, or error

Comments

@jaraco
Copy link
Member

jaraco commented Feb 24, 2023

Reported in discord, since #102018, the timing check was added to test_implied_dirs_performance and this check is frequently failing (example):

ERROR: test_implied_dirs_performance (test.test_zipfile.test_path.TestPath.test_implied_dirs_performance)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\contextlib.py", line 80, in inner
    with self._recreate_cm():
  File "D:\a\cpython\cpython\Lib\test\test_zipfile\_context.py", line 30, in __exit__
    raise DeadlineExceeded(duration, self.max_duration)
test.test_zipfile._context.DeadlineExceeded: (3.140999999999849, 3)

These failures aren't occurring on zipp, where the check has been running for years without fail.

Furthermore, the check is currently not capturing the failure case because the invocation fails to consume the generator:

zipfile.CompleteDirs._implied_dirs(data)

That indicates that the flaky failures are due to the construction of test data:

data = ['/'.join(string.ascii_lowercase + str(n)) for n in range(10000)]

Linked PRs

@jaraco jaraco added the type-bug An unexpected behavior, bug, or error label Feb 24, 2023
@jaraco
Copy link
Member Author

jaraco commented Feb 24, 2023

Some have suggested to replace the check with something that asserts the algorithmic complexity.

I've confirmed that after addressing the correctness of the check, the tests fail if I apply this diff:

diff --git a/zipp/__init__.py b/zipp/__init__.py
index f06e9fcc..6a60fc0a 100644
--- a/zipp/__init__.py
+++ b/zipp/__init__.py
@@ -63,7 +63,7 @@ def _difference(minuend, subtrahend):
     Return items in minuend not in subtrahend, retaining order
     with O(1) lookup.
     """
-    return itertools.filterfalse(set(subtrahend).__contains__, minuend)
+    return (item for item in minuend if item not in subtrahend)


class InitializedState:

I'm struggling to think how one could test the algorithmic complexity of _difference. In fact, the complexity differs based on the inputs. In the slower, generator-based version, if a set is passed as the subtrahend, the performance is comparable to the faster version. It's only when a non-set is passed does the algorithm experience quadratic time.

I think I see how one can experimentally verify linear time:

(a) measure the performance at n=5, n=10, ... n=100.
(b) test that the difference between each increment is about the same.

@zooba
Copy link
Member

zooba commented Feb 24, 2023

Experimentally verifying linear time is probably fine.

I suspect the reason it's failing now is because we run a lot of our CI tests against debug builds, rather than the fully optimised builds that were being used externally. So even measuring once for a small N and extrapolating a suitable timeout for a larger N is probably going to be fine - it's just the fixed 3 seconds is based on a different baseline.

jaraco added a commit to jaraco/cpython that referenced this issue Feb 24, 2023
@jaraco
Copy link
Member Author

jaraco commented Feb 24, 2023

I've started work in https://github.com/jaraco/jaraco.test/blob/main/jaraco/test/complexity.py to attempt to test for linear time complexity of a function, but I'm finding the test is very flaky (failing 10-20%, especially in CI).

So rather than leave CPython CI in a flaky state, I've proposed #102225 to disable the flaky check for now.

jaraco added a commit that referenced this issue Feb 24, 2023
…2225)

Disable the timeout in test_implied_dirs_performance. Workaround for #102209 until I can work out a more robust test for linearity.
jaraco added a commit to jaraco/zipp that referenced this issue Feb 24, 2023
@jaraco
Copy link
Member Author

jaraco commented Feb 24, 2023

Turns out there already exists a sophisticated library to calculate a best-fit time complexity, big-O. That library does just what's needed.

@jaraco
Copy link
Member Author

jaraco commented Feb 25, 2023

Annoyingly, the fix in zipp isn't portable to CPython (because it depends on numpy), so the solution here will be just to skip the test (unless big_o is importable). The complexity constraint will have to be maintained in zipp.

clrpackages pushed a commit to clearlinux-pkgs/pypi-zipp that referenced this issue Feb 28, 2023
…n 3.15.0

Jason R. Coombs (15):
      Add #upstream markers for filtered warnings. Add filter for platform module (ref python/cpython#100750).
      Remove reference to EncodingWarning as it doesn't exist on some Pythons.
      Revert "exclude build env from cov reporting (jaraco/skeleton#60)"
      Disable couldnt-parse warnings. Prescribed workaround for nedbat/coveragepy#1392. Fixes python/importlib_resources#279 and fixes jaraco/skeleton#56.
      Consume the _implied_dirs result, ensuring the algorithmic effect is exercised. Ref python/cpython#102209.
      Add doctests for _implied_dirs illustrating the behavior.
      Add test for getinfo on a missing. Restores 100% coverage.
      Fix EncodingWarning in doctest.
      Fix EncodingWarning in test_pickle.
      Fix coverage in conftest
      Use consume from more_itertools.
      Measure the complexity directly using the big-O library. Fixes python/cpython#102209.
      Allow sub-linear complexity (as observed on PyPy).
      Move complexity tests to their own module.
      Update changelog.
rpurdie pushed a commit to yoctoproject/poky that referenced this issue Mar 1, 2023
https://zipp.readthedocs.io/en/latest/history.html#v3-15-0

v3.15.0 (24 Feb 2023)
* gh-102209: test_implied_dirs_performance now tests measures the time
  complexity experimentally.

Reference:
python/cpython#102209

(From OE-Core rev: 1cb64a5b6a950eb7f7c72125c5741fdafe236f0b)

Signed-off-by: Tim Orling <tim.orling@konsulko.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
halstead pushed a commit to openembedded/openembedded-core that referenced this issue Mar 1, 2023
https://zipp.readthedocs.io/en/latest/history.html#v3-15-0

v3.15.0 (24 Feb 2023)
* gh-102209: test_implied_dirs_performance now tests measures the time
  complexity experimentally.

Reference:
python/cpython#102209

Signed-off-by: Tim Orling <tim.orling@konsulko.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
kraj pushed a commit to YoeDistro/poky that referenced this issue Mar 2, 2023
https://zipp.readthedocs.io/en/latest/history.html#v3-15-0

v3.15.0 (24 Feb 2023)
* gh-102209: test_implied_dirs_performance now tests measures the time
  complexity experimentally.

Reference:
python/cpython#102209

(From OE-Core rev: 98bb2929ed9a76b469f4874d7503b0fd2d3a395a)

Signed-off-by: Tim Orling <tim.orling@konsulko.com>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
daregit pushed a commit to daregit/yocto-combined that referenced this issue May 22, 2024
https://zipp.readthedocs.io/en/latest/history.html#v3-15-0

v3.15.0 (24 Feb 2023)
* gh-102209: test_implied_dirs_performance now tests measures the time
  complexity experimentally.

Reference:
python/cpython#102209

(From OE-Core rev: 1cb64a5b6a950eb7f7c72125c5741fdafe236f0b)

Signed-off-by: Tim Orling <tim.orling@konsulko.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this issue Sep 10, 2024
python#102225)

Disable the timeout in test_implied_dirs_performance. Workaround for python#102209 until I can work out a more robust test for linearity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants