get: implement --show-url to display only url/path to remote #3156

skshetry · 2020-01-15T12:39:44Z

Closes #2994.

From the product requirements, it deviates on the following issue:

For now, it does not support showing URLs for multiple paths.
So, we can only do dvc get <url> <path> --show-url for a single path, so as
to make it compatible with dvc get.
It also does not support specifying remote at the moment, so as to not introduce new flags. (get: --show-url does not support specifying remotes #3183)
It does not work with the file inside of a directory (i.e. dir/file). Limitation of dvc.api.get_url(). Requires adding directory granularity support. (api: get_url does not support links for granular file #3180)
```
➜ dvc get . dir/file --show-url
ERROR: failed to show url - unable to find DVC-file with output 'dir/file'
```
How to show path/URL to the directory? Currently, it only shows .dir file. (api: get_url() returns path to .dir for directory #3182)
```
➜ dvc get . dir --show-url
../tmp.fKEyzBMWge/12/11325135bf45fe5f67efa975110a57.dir
```
Same is the case with dvc get (cc @jorgeorpinel for docs regarding dvc.api.get_url()).

It does not work with non-dvc repos (i.e. git only repos). So, the following won't work (api/get: cannot show url for git only repos #3181):

➜ dvc get https://github.com/schacon/cowsay install.sh --show-url 
ERROR: failed to show url - URL 'https://github.com/schacon/cowsay' is not a dvc repository.

Custom revision does not work for the local repo.
Not fixing at the moment. api/get: local repo with custom rev is not supported #3179 tracks this.

➜ dvc get . foo --rev=HEAD@{5.days.ago} # works
➜ dvc get . foo --rev=HEAD@{5.days.ago} --show-url
ERROR: unexpected error - Custom revision is not supported for local repo

Example Usage

➜ dvc get . foo --show-url
../tmp.uJVvn5ZaLY/d3/b07384d113edec49eaa6238ad5ff00
➜ dvc get https://github.com/iterative/dataset-registry.git get-started/data.xml --rev=HEAD@{5.days.ago} --show-url
https://remote.dvc.org/dataset-registry/a3/04afb96060aad90176268345e10355

EDIT: Made changes to format from <path> <url> previously to just <url>.

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
cmd ref: document get --show-url option dvc.org#930
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

TODO

Edgecases

get: implement --show-url to display only url/path to remote #3156 (comment)

skshetry · 2020-01-15T14:59:50Z

Some subtle difference that I noticed with --show-url flags vis-a-vis without flags:

get has path as a positional argument. I don't want to change it to have nargs=*. Is it okay to have get <url> <single_path> --show-url?
dvc get . foo --rev=HEAD~ works, but, dvc get . foo --rev=HEAD~ --show-url does not work because of assertion in _make_repo().

So, this is currently failing:

def test_get_url_with_rev(tmp_dir, erepo_dir):
    with erepo_dir.chdir():
        erepo_dir.dvc_gen("foo", "foo")
        assert main(["get", ".", "foo", "--rev", "HEAD", "--show-url"]) == 0

dvc/command/get.py

efiop · 2020-01-15T15:10:58Z

dvc/command/get.py

I guess good question here is whether or not --show-url should also become a part of repo.get. Hm... For now, we can leave it as is, since api has get_url anyways. 👍

Yes, I wanted to refactor get_url and make that part of the internals, but, I left it for now.

Though, we could consider splitting it into _get_url and _show_url here, so that we could test them separately without invoking main each time. Kinda like we do in pipeline show.

I refactored it a bit to make it like pipeline show, but, I didn't see any tests where we directly make use of Cmd* commands (we do check for isinstance in few places though). pipeline show tests also calls main.

dvc/command/get.py

ghost

@skshetry , looks good!

I don't like the idea of having --show-url instead of a separate command.
Specially because get is pretty ambiguous 😓 (get what?)
It could be like get --url, but colliding two verbs like get + --show feels weird.

I know that you didn't make the decision, just advocating for something else) (cc: @shcheklein)

Is get --show-url | wget the same as get?

shcheklein · 2020-01-15T23:20:29Z

@MrOutis I think the short version --url will be even more confusing. get --url - feels like we provide an URL to get from or something. The whole thing is not ideal, but I still don't feel we should add a global command just for this. Also, like @jorgeorpinel mentioned we can move this to the future dvc list - it probably makes even more sense than dvc get.

ghost · 2020-01-15T23:22:55Z

@shcheklein , indeed! let's roll with --show-url and then move it to list

jorgeorpinel · 2020-01-15T23:31:26Z

Is get --show-url | wget the same as get?

Only for HTTP remotes.

ghost · 2020-01-16T01:19:01Z

Only for HTTP remotes.

So, if I use it for S3 it won't give me a usable URL?

efiop · 2020-01-16T01:57:12Z

@MrOutis It will, but not usable with wget 🙂 I think

dvc get .. --show-url | dvc get-url

would work though 😄 Checkmate, wget!

skshetry · 2020-01-16T05:53:57Z

dvc get .. --show-url | dvc get-url

@efiop, do we support reading from stdin? Couldn't find any, and above command does not work, because we don't read from stdin (and, I need to change format of the output).

Workaround for now is following:

dvc get .. --show-url | xargs dvc get-url

Is get --show-url | wget the same as get?

The above also does not seem to work unless we use xargs or wget -i -.

But, I have to change the output from <path> <url> to just <url>, as it works for only one path at the moment.

efiop · 2020-01-16T09:22:10Z

Good point @skshetry ! So wget behaves the same, let's keep the same behavior in get-url for now as well 🙂

skshetry · 2020-01-16T09:29:41Z

Good point @skshetry ! So wget behaves the same, let's keep the same behavior in get-url for now as well

You mean, introducing - arg? Or, leave as it is (like, plain wget)?

efiop · 2020-01-16T10:06:02Z

@skshetry I meant just leaving it as is for now, indeed 🙂

tests/func/test_get.py

pared

Left few comments under tests

dvc/command/get.py

skshetry · 2020-01-16T13:46:39Z

dvc/api.py

+            remote_obj = _repo.cloud.get_remote(remote)
+            return str(remote_obj.checksum_to_path_info(out.checksum))
+    except NotDvcRepoError:
+        raise UrlNotDvcRepoError(repo)


I'll raise this same error in _make_repo in a separate PR. Just keeping it here for now.

tests/func/test_api.py

On windows, `str(exc_info)` is returning escaped path, eg: '<ExceptionInfo UrlNotDvcRepoError("URL \'C:\\\\Users\\\\travis\\\\AppData\\\\Local\\\\Temp\\\\pytest-of-travis\\\\pytest-0\\\\popen-gw2\\\\test_get_url_git_only_repo_thr0\' is not a dvc repository.") tblen=2>' This commit just replaces the double slashes with single one. Bit of a hack, I know.

tests/func/test_get.py

pared

Minor suggestions, not required, besides that LGTM.

tests/unit/command/test_get.py

Co-Authored-By: Paweł Redzyński <pawelredzynski@gmail.com>

efiop · 2020-01-17T07:44:57Z

dvc/command/get.py

+    get_parser.add_argument(
+        "--show-url",
+        action="store_true",
+        help="Returns path/url to the location in remote for specified path",


Could use the description from https://github.com/iterative/dvc/pull/3130/files

Though, looks like it is still up for discussion, so i'm sure we will modify it while working on the docs. Let's keep it as is for now 👍

Ah yeah I would change this description, its confusing. Sorry, didn't notice before merge...

efiop

Looks great! Please see one minor comment above.

efiop · 2020-01-17T07:47:13Z

@skshetry Also, need to not forget about docs 🙂

pared

LGTM

efiop

Thanks! 🎉

jorgeorpinel

@skshetry sorry didn't get to review this before merge but there are some language improvements from my side, tied to iterative/dvc.org/pull/936. Thanks

dvc/api.py

jorgeorpinel · 2020-01-20T22:54:55Z

dvc/command/get.py

+            )
+            logger.info(url)
+        except DvcException:
+            logger.exception("failed to show url")


Capitalization: "Failed to show URL" Maybe end in period . ?
Depends on whether it's printed as part of a larger message.

@jorgeorpinel, the lowercase seems to be used everywhere in the codebase. And, because this is a logger.exception, our formatter handles adding extra information from raised exception besides this particular logging message. Notice the messages on following examples:

➜ dvc get https://github.com/schacon/cowsay install.sh --show-url ERROR: failed to show url - URL 'https://github.com/schacon/cowsay' is not a dvc repository. ➜ dvc get . dir/file --show-url ERROR: failed to show url - unable to find DVC-file with output 'dir/file'

failed to show url - URL it looks strange though :) First it proves that the lowercase seems to be used everywhere is not true (and it should not be, typically, acronyms and initialisms are written in all capital letters to distinguish them from ordinary words. - NASA, URL, DVC, etc). Second, you have a repetition here. Repetition is probably fine if reasons vary and you can't actually control them.

@shcheklein, it's not something that I add, but, I'm utilizing the fact that something will be there. So, it's more like "(what failed) - (the reason)".

And, yes, regarding lowercase, I just looked into it and it's not everywhere for other cases of logging, but for exceptions, lowercase is used everywhere.

because this is a logger.exception, our formatter handles adding extra information

Thanks, good to know this.

lowercase seems to be used everywhere is not true...

➕ 1️⃣ No need to use bad grammar on purpose because of existing bad grammar.

failed to show url - URL it looks strange though :) ...

it's more like "(what failed) - (the reason)".

So then the reason part in this case should start directly with http://... – but that's not part of this PR...

the reason part in this case should start directly with http://... – but that's not part of this PR

OK I changed this in ec0c39a#diff-3b2745249f7139953ab4b45b382e69e9 but this whole conversation kind of opened a can of worms and that PR (#3220) grew quite a bit 😅 Hoping core team doesn't flip out.

but for exceptions, lowercase is used everywhere

Thanks for the tip. I may review some of these too 😋

dvc/command/get.py

+    get_parser.add_argument(
+        "--show-url",
+        action="store_true",
+        help="Returns path/url to the location in remote for specified path",


tests/func/test_api.py

jorgeorpinel · 2020-01-23T01:50:22Z

UPDATE: I'm addressing my own feedback in #3220.

skshetry self-assigned this Jan 15, 2020

skshetry force-pushed the get-url-from-cache branch from f466bed to 97d57ea Compare January 15, 2020 15:02

efiop reviewed Jan 15, 2020

View reviewed changes

dvc/command/get.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

efiop reviewed Jan 15, 2020

View reviewed changes

dvc/command/get.py Outdated Show resolved Hide resolved

efiop reviewed Jan 15, 2020

View reviewed changes

skshetry force-pushed the get-url-from-cache branch from 97d57ea to e99607f Compare January 15, 2020 15:13

skshetry marked this pull request as ready for review January 15, 2020 15:18

skshetry changed the title ~~[WIP] get: implement --show-url to display only url/path to remote~~ get: implement --show-url to display only url/path to remote Jan 15, 2020

skshetry requested review from a user, efiop and pared January 15, 2020 15:30

efiop reviewed Jan 15, 2020

View reviewed changes

dvc/command/get.py Outdated Show resolved Hide resolved

ghost approved these changes Jan 15, 2020

View reviewed changes

pared reviewed Jan 16, 2020

View reviewed changes

tests/func/test_get.py Outdated Show resolved Hide resolved

pared reviewed Jan 16, 2020

View reviewed changes

tests/func/test_get.py Outdated Show resolved Hide resolved

pared reviewed Jan 16, 2020

View reviewed changes

tests/func/test_get.py Outdated Show resolved Hide resolved

pared suggested changes Jan 16, 2020

View reviewed changes

efiop reviewed Jan 16, 2020

View reviewed changes

dvc/command/get.py Outdated Show resolved Hide resolved

efiop reviewed Jan 16, 2020

View reviewed changes

dvc/command/get.py Outdated Show resolved Hide resolved

skshetry force-pushed the get-url-from-cache branch from b7ce015 to aa7119a Compare January 16, 2020 13:43

skshetry requested review from a user, efiop and pared January 16, 2020 13:45

skshetry commented Jan 16, 2020

View reviewed changes

tests/func/test_api.py Outdated Show resolved Hide resolved

pared reviewed Jan 16, 2020

View reviewed changes

tests/func/test_get.py Show resolved Hide resolved

skshetry requested a review from Suor January 16, 2020 16:28

unit: test: check output of get --show-url command

02a73a0

pared reviewed Jan 16, 2020

View reviewed changes

tests/unit/command/test_get.py Outdated Show resolved Hide resolved

tests/unit/command/test_get.py Outdated Show resolved Hide resolved

tests/unit/command/test_get.py Outdated Show resolved Hide resolved

test: refactor, apply suggestion from @pared

98f6d56

Co-Authored-By: Paweł Redzyński <pawelredzynski@gmail.com>

efiop requested a review from pared January 17, 2020 07:41

efiop reviewed Jan 17, 2020

View reviewed changes

efiop suggested changes Jan 17, 2020

View reviewed changes

pared approved these changes Jan 17, 2020

View reviewed changes

efiop approved these changes Jan 17, 2020

View reviewed changes

efiop merged commit bdd1d61 into iterative:master Jan 17, 2020

skshetry deleted the get-url-from-cache branch January 17, 2020 08:15

This was referenced Jan 17, 2020

cmd ref: document get --show-url option iterative/dvc.org#930

Closed

api/get: local repo with custom rev is not supported #3179

Closed

api: get_url does not support links for granular file #3180

Closed

completion: completion for --show-url #3184

Closed

jorgeorpinel reviewed Jan 20, 2020

View reviewed changes

This was referenced Jan 20, 2020

cmd ref: get: document --show-url flag iterative/dvc.org#936

Merged

get: copy edits aroud --show-url, "dvc", et al. #3220

Merged

Uh oh!

get: implement --show-url to display only url/path to remote #3156

get: implement --show-url to display only url/path to remote #3156

Uh oh!

Conversation

skshetry commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example Usage

TODO

Edgecases

Uh oh!

skshetry commented Jan 15, 2020

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

shcheklein commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 15, 2020

Uh oh!

jorgeorpinel commented Jan 15, 2020

Uh oh!

ghost commented Jan 16, 2020

Uh oh!

efiop commented Jan 16, 2020

Uh oh!

skshetry commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiop commented Jan 16, 2020

Uh oh!

skshetry commented Jan 16, 2020

Uh oh!

efiop commented Jan 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efiop Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

skshetry commented Jan 15, 2020 •

edited

Loading

shcheklein commented Jan 15, 2020 •

edited

Loading

skshetry commented Jan 16, 2020 •

edited

Loading

efiop Jan 17, 2020 •

edited

Loading

jorgeorpinel Jan 20, 2020 •

edited

Loading

jorgeorpinel Jan 20, 2020 •

edited

Loading

skshetry Jan 21, 2020 •

edited

Loading

jorgeorpinel Jan 23, 2020 •

edited

Loading

jorgeorpinel Jan 23, 2020 •

edited

Loading