-
Notifications
You must be signed in to change notification settings - Fork 135
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
Use Scope for VCS #445
Merged
Merged
Use Scope for VCS #445
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rned (#443) * lazy_image: fix cache collisions leading to unrelated data being returned Currently, the key used to look up the cached image is based on a hash of a tuple containing `id(self)`, `path`, and `loader`. This means there are two situations in which a `lazy_image` can look up the wrong data: * If there previously existed another `lazy_image` object whose `self` and `loader` had the same object IDs as the current `lazy_image`'s `self` and `loader`. This is possible, because deleted objects' IDs can be reused. * If a hash collision occurs between the current `lazy_image`'s tuple and some other's. Fix it by using a weak reference to `self` as the key instead. Different weak references will only compare equal if they point to the same object. This will only work correctly if the loader and path of a `lazy_image` are not modified after creation. I don't think there are any use cases for modifying them (and there are no instances of that happening in the codebase), so it shouldn't be an issue. To reduce the temptation of client code to modify these fields, mark them as private. Modifying the `cache` field should not cause issues, but just in case, make it private as well. * Add a changelog entry for the lazy_image cache collision fix
* Replace rollback with scope * Add scope_add() function * Update tests * Replace rollback uses * Update changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Depends on #444
Introduces resource management for the
Project
class. Fixes file removal issues on Windows. Improves test performance by disabling DVC analytics sending.How to test
Checklist
develop
branchLicense
Feel free to contact the maintainers if that's a concern.