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

cmd ref: freeze/unfreeze 1.0 and improvements #1493

Merged
merged 6 commits into from
Jun 25, 2020
Merged

cmd ref: freeze/unfreeze 1.0 and improvements #1493

merged 6 commits into from
Jun 25, 2020

Conversation

jorgeorpinel
Copy link
Contributor

Continuation of #1488

@shcheklein shcheklein temporarily deployed to dvc-landing-1-0-freeze-ku3cfi5 June 24, 2020 03:15 Inactive
@jorgeorpinel jorgeorpinel requested a review from pared June 24, 2020 03:16
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-1-0-freeze-ku3cfi5 June 24, 2020 03:16 Inactive
Comment on lines +26 to +29
Note that <abbr>import stages</abbr> are frozen by default. Use `dvc update` to
update the corresponding <abbr>data artifacts</abbr> from the external data
source. [Unfreeze](/doc/command-reference/unfreeze) them before using
`dvc repro` on a pipeline that needs their outputs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this paragraph correct? Are import stages frozen by default, but they can be unfrozen so that repro re-downloads their outputs?

Copy link
Member

Choose a reason for hiding this comment

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

It's tricky and not very consistent (import vs import-url) - @efiop could you clarify how does it work these days? Also, we should start getting rid of the "import stage" terminology. At least while they stay out of dvc.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe @pared knows. I can try this later if not.

Copy link
Contributor

@pared pared Jun 25, 2020

Choose a reason for hiding this comment

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

Is this paragraph correct? Are import stages frozen by default, but they can be unfrozen so that repro re-downloads their outputs?

Seems to be true. Example:

#!/bin/bash

rm -rf repo other
mkdir repo other
main=$(pwd)

echo data >> data

pushd other

git init --quiet && dvc init --quiet
echo data >> data
dvc add data

git add -A
git commit -am "ble"
popd
pushd repo

git init --quiet && dvc init --quiet

dvc import-url $main/data imp-url
dvc import $main/other data -o imp 

popd
pushd other
echo modify >> data
dvc commit -f data.dvc
git commit -am "something"

popd
echo modify >> data

pushd repo
dvc status

dvc unfreeze imp-url.dvc
dvc unfreeze imp.dvc
dvc repro imp-url.dvc imp.dvc

It's tricky and not very consistent (import vs import-url) - @efiop could you clarify how does it work these days? Also, we should start getting rid of the "import stage" terminology. At least while they stay out of dvc.yaml.

the main difference between import-url and import results is how dvc status will react to them - in case of import-url we will get warning that it is frozen, in case of import-ed file the status will be checked and returned appropriately.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jun 25, 2020

Choose a reason for hiding this comment

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

Thanks @pared! So this paragraph is correct so the main point here is resolved. However we have these 2 side issues that may be worth exporting somewhere else:

we should start getting rid of the "import stage" terminology. At least while they stay out of dvc.yaml.

@shcheklein Idk. import .dvc files have both deps and outs so they're full stages. Added a checkbox about this to #608 for now.

import-url we will get warning that it is frozen, in case of import-ed file the status will be checked

@pared do you think both should behave the same though? Maybe both like regular import, just check the status, no warning. Or both warn + check status properly. If so maybe we should open a separate issue about this in the core repo.

Copy link
Member

Choose a reason for hiding this comment

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

@pared

the main difference between import-url and import results is how dvc status will react to them - in case of import-url we will get warning that it is frozen, in case of import-ed file the status will be checked and returned appropriately.

should it be a bug?

have both deps and outs so they're full stages.

deps and outs do not necessarily mean that something is a stage. It's an implementation detail. Probably a leftover for the backward compatibility. From a user perspective it's just an input data (similar to dvc add) to my mind. It's imported, yes, but it does not make any difference I think.

I agree that it is a separate discussion though and a separate ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an implementation detail... From a user perspective it's just an input data (similar to dvc add)... It's imported, yes, but it does not make any difference I think.

Yeah, you're right. We should get rid of the term then 🙂 (noted in #608)

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-1-0-freeze-ku3cfi5 June 24, 2020 04:48 Inactive
content/docs/command-reference/freeze.md Outdated Show resolved Hide resolved
content/docs/command-reference/unfreeze.md Outdated Show resolved Hide resolved
@pared
Copy link
Contributor

pared commented Jun 24, 2020

I will take on changing the messages in core

@jorgeorpinel jorgeorpinel requested a review from pared June 24, 2020 17:25
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-1-0-freeze-ku3cfi5 June 24, 2020 17:25 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-1-0-freeze-ku3cfi5 June 24, 2020 17:30 Inactive
@jorgeorpinel
Copy link
Contributor Author

@pared do you approve the changes now?

@jorgeorpinel jorgeorpinel requested a review from shcheklein June 25, 2020 19:20
@shcheklein shcheklein merged commit 87e83e1 into master Jun 25, 2020
@pared
Copy link
Contributor

pared commented Jun 29, 2020

@pared do you approve the changes now?
@jorgeorpinel, sorry for delay, I do

@shcheklein shcheklein deleted the 1.0/freeze branch June 30, 2020 22:44
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.

3 participants