Skip to content

dvc: move repo lock from CLI to API #2519

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 21, 2019
Merged

dvc: move repo lock from CLI to API #2519

merged 1 commit into from
Sep 21, 2019

Conversation

@efiop
Copy link
Contributor

@efiop efiop commented Sep 21, 2019 •
edited
Loading

For the longest time we've had repo.lock, but were only using it from
CLI when running specific command, instead of simply properly taking it
on API operations. This patch takes care of that.

Related to #755

  • Have you followed the guidelines in our
    Contributing document?

  • Does your PR affect documented changes or does it add new functionality
    that should be documented? If yes, have you created a PR for
    dvc.org documenting it or at
    least opened an issue for it? If so, please add a link to it.

@Suor
Copy link
Contributor

Suor commented Sep 21, 2019

I don't think flufl locks are reentrable, you will need to make extra effort to provide that.

@efiop
Copy link
Contributor Author

efiop commented Sep 21, 2019 •
edited
Loading

@Suor Maybe I'm missing something, but our locks were never reentarable. Or are you talking about us wanting that in the upcoming patches related to stage locks?

@Suor
Copy link
Contributor

Suor commented Sep 21, 2019

I see you don't lock pull, a composite command. Is it safe though? Some other call might get in between and remove freshly downloaded files.

@Suor
Copy link
Contributor

Suor commented Sep 21, 2019

Or target stage might be modified/removed between the locks.

@efiop
Copy link
Contributor Author

efiop commented Sep 21, 2019 •
edited
Loading

@Suor Great points! I'll adjust that using intermediate _fetch and _checkout, as it was in one of the earliest versions of this. Or might find a better way. Thank you! 🙂

@Suor
Copy link
Contributor

Suor commented Sep 21, 2019

@efiop I would say making locks or just @locked decorator reentrable would be clearer solution.

@efiop
Copy link
Contributor Author

efiop commented Sep 21, 2019

@Suor Btw found the bug in checkout along the way. I guess that is why you were talking that we are collecting stages many times. Will send a fix for that too.

@efiop
Copy link
Contributor Author

efiop commented Sep 21, 2019 •
edited
Loading

@Suor I would really preferer not to use reenterable locks as long as I can. Non-reenterable locks help find bugs, as they pose very strict requirements. But I'll definitely take a look. Thanks 🙂

@efiop
dvc: move repo lock from CLI to API
9d6ca1e 
For the longest time we've had `repo.lock`, but were only using it from
CLI when running specific command, instead of simply properly taking it
on API operations. This patch takes care of that.
@efiop efiop requested a review from Suor September 21, 2019 02:34
Suor
Suor suggested changes Sep 21, 2019
View reviewed changes
Copy link
Contributor

@Suor Suor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what kind of bugs non-reentrable locks will prevent. Not permitting calling one api call form another one? Do we want that?

The code complications are not big for now, though.

dvc/main.py
ret = cmd.run_cmd()
ret = cmd.run()
except LockError:
logger.exception("failed to lock before running a command")
Copy link
Contributor

@Suor Suor Sep 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Should we show more descriptive and actionable message? Like what is happening, i.e. that some concurrent dvc command is blocking. And if that is not so then what lock files should people remove.

Copy link
Contributor Author

@efiop efiop Sep 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@Suor Sure. This is the old one that I've just moved. Created an issue for that one #2520

@efiop efiop mentioned this pull request Sep 21, 2019
Closed
@efiop
Copy link
Contributor Author

efiop commented Sep 21, 2019

@Suor The initial idea was that it would protect from re-collecting the DAG, but, as we've seen with checkout for multiple targets, it didn't help much and we indeed do multiple self.stage() 🙁 As you've noted during one of the previous reviews for stage unlock patch, there using reentrant locks would be much more useful. So let's reconsider introducing them there, and in this PR just keep the old behaviour as is.

@efiop efiop requested a review from Suor September 21, 2019 03:28
Suor
Suor approved these changes Sep 21, 2019
View reviewed changes
Copy link
Contributor

@Suor Suor left a comment

Choose a reason for hiding this comment

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

Looks good

@efiop efiop merged commit 23fc266 into iterative:master Sep 21, 2019
@efiop efiop mentioned this pull request Sep 24, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Suor Suor Suor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@efiop @Suor