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

Add dvc run --deterministic option. #1400

Merged
merged 21 commits into from
Dec 4, 2018
Merged

Conversation

vernt
Copy link
Contributor

@vernt vernt commented Dec 2, 2018

Tests currently failing. I'm not familiar with the test tooling to diagnose the situation.

Related to #1234

efiop
efiop previously requested changes Dec 2, 2018
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Hi @vernt !

A few suggestions with fixes for the errors from the CI logs. You can click on them to merge them into your patch. Lets do that and I will continue the review from there.

dvc/project.py Show resolved Hide resolved
dvc/stage.py Outdated Show resolved Hide resolved
dvc/stage.py Outdated Show resolved Hide resolved
@efiop
Copy link
Contributor

efiop commented Dec 2, 2018

Hi @vernt !

I've fixed those small issues myself. Waiting for CI results.

@efiop efiop dismissed their stale review December 2, 2018 16:12

Fixed CI errors.

@efiop
Copy link
Contributor

efiop commented Dec 2, 2018

Hi @vernt !

I'll submit my test additions in a moment on top of your changes. Can you confirm that this patch works for you during the manual testing?

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop
Copy link
Contributor

efiop commented Dec 2, 2018

One more test coming in a moment. So far so good 🙂

We should check command, as well as any other fields. E.g. `locked`.

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop
Copy link
Contributor

efiop commented Dec 2, 2018

@vernt I've added a few things as well as tests for them. Please take a look, maybe you want to add/change something. Please ping me when you think it is ready to be merged. Thanks a lot for the PR! 🎉

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop
Copy link
Contributor

efiop commented Dec 4, 2018

NOTE: need to implement #1373 before merging this PR.

Fixes iterative#1373

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>
@efiop efiop changed the title [WIP] Add dvc run --deterministic option. Add dvc run --deterministic option. Dec 4, 2018
@efiop efiop merged commit 9f4dd2d into iterative:master Dec 4, 2018
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.

None yet

2 participants