-
Notifications
You must be signed in to change notification settings - Fork 29
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 invalidate obj on close #54
Conversation
…ontinue to work after a commit is made
…ugs leftover in the test suite now work with the changes implemented here
b567aee
to
69ec6f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -431,7 +443,7 @@ def reset_staging_area(self): | |||
commit_hash=head_commit) | |||
|
|||
logger.info(f'Hard reset completed, staging area head commit: {head_commit}') | |||
self.__setup() | |||
self.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rlizzo Personally, I don't have any sentiments towards this but I have a feeling that closing the checkout implicitly would be confusing for users. Just wanted you to have a second thought about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'm going to mark it as a TODO for now. There's a bit more logistics that go into handling a reset since we need to invalidate some objects and potentially not others. For now I'm ok with it though.
Motivation and Context
Why is this change required? What problem does it solve?:
A full overview of the problem is provided in: #53 (comment). However, in brief:
write-enabled checkout
(and onlywrite-enabled
) would inadvertently invalidate the oldweakproxy
references which had been assigned to it, before thecheckout.close()
method was formally called.checkout.commit()
andcheckout.reset_staging_area()
were called.checkout.__setup()
, replaced the only strong references to previously"proxied"
dataset
ormetadata
objects with new versions.If it fixes an open issue, please link to the issue here:
Description
Describe your changes in detail:
"__setup"
should probably only be called once upon actual class "setup"..., the problem calls were just wholesale removed fromcommit()
andreset_stating_area()
.commit()
, we actually do need to perform some setup work for the backend file-stores (mainly to open and close file handles so symlinks can be properly identified). This work was pushed into theDatasetDataWriter
class which actually performed that work natively, and this is arguably cleaner than the previous implementation. No changes were needed formetadata
if we don't try to rebuild the entire object, it just works :)reset_staging_area()
, I thought the best choice would be to just have that operation close the checkout in full just before itreturns
. It's probably best for a user to fully reset their environment after wiping out the staging area in full, and that is the most effective way to ensure no leftover state is hanging around after this is called.Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Is this PR ready for review, or a work in progress?
How Has This Been Tested?
Put an
x
in the boxes that apply:Checklist: