Skip to content

Conversation

@jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Sep 23, 2020

@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 23, 2020 18:29 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 23, 2020 21:09 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 23, 2020 21:12 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 23, 2020 21:54 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 23, 2020 22:22 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 25, 2020 21:17 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 25, 2020 21:18 Inactive
Comment on lines 105 to 106
```yaml
< < < < < < < HEAD
md5: 263395583f35403c8e0b1b94b30bea32
=======
md5: 520d2602f440d13372435d91d3bfa176
> > > > > > > branch
frozen: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@efiop another quick Q: this was added accidentally right? As .dvc files no longer have a top md5 field. Just double checking

Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgeorpinel This is an import file, so they do have the md5 and so this change is wrong 🙁

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Sep 26, 2020

Choose a reason for hiding this comment

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

  • OK np I'll revert it.

Why do import .dvc files need a top md5 though? We may need to document this in https://dvc.org/doc/user-guide/dvc-files-and-directories or in the import(-url) refs. at least. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably will be removing it in the future, it has been in discussions for a while now.

Indeed, looks like https://dvc.org/doc/user-guide/dvc-files-and-directories doesn't have imports at all 🙁

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 25, 2020 21:53 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 25, 2020 22:03 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 26, 2020 03:53 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 26, 2020 03:59 Inactive
@jorgeorpinel
Copy link
Contributor Author

Sorry, this PR got too large somehow (I had an irregular week). But 9/16 files only have small indentation or typo changes. Hopefully it's not too much to review, but lmk if I should split it. Thanks

< < < < < < < HEAD
rev_lock: f31f5c4cdae787b4bdeb97a717687d44667d9e62
=======
= = = = = = =
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that most IDEs will think it's an actual conflict if we save it like that. Maybe even GitHub or some code analyzers will pick it up as a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And we can't use HTML encoding since it's in a md code block.

And then `dvc update` the `.dvc` file to download the latest data from its
original source.

> Note that updating will bring in the latest version of the data found in its
Copy link
Contributor

Choose a reason for hiding this comment

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

@efiop could you confirm this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, correct.

Copy link
Contributor

@shcheklein shcheklein 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, some comments to fix, otherwise ready to merge

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-gygjvh75jusb September 26, 2020 05:11 Inactive
@jorgeorpinel
Copy link
Contributor Author

Thanks! Merging this but I can do a quick followup if any of the recent comments above require more iterations to be addressed.

@jorgeorpinel jorgeorpinel merged commit 11ca7bd into master Sep 26, 2020
Copy link
Contributor Author

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

A couple things I noticed based on the changes above:

Comment on lines +162 to +168
Let's see an example using SSH. First, register and configure the remote:

```dvc
$ dvc remote add myssh ssh://myserver.com
$ dvc remote modify --local myssh user myuser
$ dvc remote modify --local myssh password mypassword
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me realize our SSH examples for remote add/modify may be misleading (in other docs) because we sometimes use ssh://user@example.com... when adding, and then mention about modifying the credentials or even show an example of remote modify example user ... but I don't know if that will work correctly with DVC. Need to check...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried testing this but can't because of treeverse/dvc#4712.

Comment on lines +173 to 175
Now, use an alias to this remote when defining the stage:

```dvc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also realized this doc only uses dvc run to define stages and doesn't show the resulting dvc.yaml files (except for import(-url) examples. Maybe we shouldn't even use dvc run in most of these examples? Since manually writing dvc.yaml is the recommended way.

But then again you can't easily write an external dependency in dvc.yaml ... Hmmm 🤔

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.

sync: mention that fetch and pull re-import data

3 participants