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

Fix cache bug if index version is too new #1644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

hunse
Copy link
Collaborator

@hunse hunse commented Sep 30, 2020

Motivation and context:

If the cache version is too new, we should just ignore the cache. Currently, there's an error because it tries to use the index when it's None.

How has this been tested?

I added a test.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

@hunse
Copy link
Collaborator Author

hunse commented Sep 30, 2020

This was caused by 4caba86, because we changed a number of except Exception statements to more specific exceptions, but obviously missed catching the exception that causes this problem. We might want to consider switching a few of those back and just disabling the pylint warning (in cache.py specifically).

Copy link
Collaborator

@jgosmann jgosmann left a comment

Choose a reason for hiding this comment

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

I think this will print a "Decoder cache could not acquire lock and was deactivated." warning even though it might be a version mismatch. While this broke prior to this PR, it might make sense to fix that as part of this PR?


# Write index with super large version numbers
with open(os.path.join(cache_dir, CacheIndex._INDEX), "wb") as f:
pickle.dump((1000, 1000), f)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use (CacheIndex.VERSION + 1, pickle.HIGHEST_PROTOCOL + 1) to make it perfectly clear that these number are supposed to be higher than the current version numbers and avoid the "magic" numbers (but this is not a blocker to me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants