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

disable cache for all tests except test_cache.py and benchmark tests #886

Closed
wants to merge 4 commits into from

Conversation

lapp0
Copy link
Contributor

@lapp0 lapp0 commented May 10, 2024

Fixes #853

@lapp0 lapp0 changed the title disable cache for all tests except test_cache tests disable cache for all tests except test_cache.py tests May 10, 2024
@lapp0 lapp0 changed the title disable cache for all tests except test_cache.py tests disable cache for all tests except test_cache.py and benchmark tests May 10, 2024
@@ -7,7 +7,10 @@
import cloudpickle
from diskcache import Cache

_caching_enabled = True
if os.environ.get("OUTLINES_DISABLE_CACHE", "") in ("", "0"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need an environment variable for this?

Also, can't we use the existing disable_cache function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be beneficial to introduce an environment variable that allows users disable cache, even outside of pytest.

I could also simply make the disable_cache fixure patch this variable and have no changes to caching.py if we don't want to expose this.

Copy link
Contributor

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To move this forward, we need a description of the approach and some justification(s) for why we should (or need to) use it over the alternatives (e.g. see the approaches in the test suite).

@lapp0
Copy link
Contributor Author

lapp0 commented May 10, 2024

We need a general solution that is applied to all test cases in tests/conftest.py. This will ensure new test cases don't accidentally reintroduce the issue you highlighted.

I agree about the environment variable being a problem. The cleanest solution is to clear the cache before each individual test run. This won't require any special handling for benchmarks or test_cache.py

@brandonwillard
Copy link
Contributor

We need a general solution that is applied to all test cases in tests/conftest.py. This will ensure new test cases don't accidentally reintroduce the issue you highlighted.

I agree about the environment variable being a problem. The cleanest solution is to clear the cache before each individual test run. This won't require any special handling for benchmarks or test_cache.py

Clearing the cache is another necessarily "global" approach with similar drawbacks.

Ideally, we would have something that works, still allows the tests to be run in parallel, and introduces a minimal amount of new code/logic (even if said code can be multi-purposed).

@lapp0
Copy link
Contributor Author

lapp0 commented May 10, 2024

Thanks for reviewing. Indeed, we want to ensure the code remains robust. I'll close this for now.

@lapp0 lapp0 closed this May 10, 2024
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.

Disable regex caching in tests
2 participants