Display path relative to temporary repo when get/import-ing files by skshetry · Pull Request #2798 · treeverse/dvc

skshetry · 2019-11-16T02:17:45Z

❗ 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.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addresses. 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. 🙏

Description

This PR fixes #2605. This will print the relative path of files being downloaded in the console.

efiop · 2019-11-16T09:35:04Z

Thanks for the PR @skshetry ! Just a heads up: some tests failed, please take a look at travis logs. 🙂

skshetry · 2019-11-16T09:51:54Z

@efiop, this test is failing for SSH/HDFS/s3/GS as they are remote. Others are passing.
Should I just separate this test (and, keep it only for local)?

I have separated the test for now. Tell me, I feel, I have done something wrong here 😃

Now the three old tests are not tested for other remotes than local.

Considering all the test complications I suggest not touching existing ones, but adding a separate pytest-style test to check output stringification for an output in external repo.

Now the three old tests are not tested for other remotes than local.

They do run, as the above TestCase is not changed at all. Or, did you mean this specific example?

Considering all the test complications I suggest not touching existing ones, but adding a separate pytest-style test to check output stringification for an output in external repo.

You mean, for each remote separately? Please, clarify.

They do run, as the above TestCase is not changed at all. Or, did you mean this specific example?

Oh, I see, I was inattentive. And OutputLOCALBase not really used confused me.

You mean, for each remote separately? Please, clarify.

You don't need to do anything for each remote. You are only changing local one and thus only need to add tests for it. You can do that without touching existing tests at all and instead create two new test functions to check the specific changes you introduced.

@skshetry @Suor is saying that you've modified TestOutputLOCAL which is used in tests for other remotes, hence why the tests are failing and why not all tests are now running for those remotes. You need to not touch the old tests, but create a new one pytest-style test to test that stringification works properly for local output when you are in and outside of the repo. 🙂

Suor · 2019-11-16T10:28:04Z

This pattern is used over and over in our code. This is probably a good time to extract this into some path_isin() utility.

Suor · 2019-11-16T10:31:29Z

Now the three old tests are not tested for other remotes than local.

Suor · 2019-11-16T10:38:58Z

The original issue this PR aims to solve was about outputs in external repo and thus outside it. Testing with stage of the same repo might not cover that or might break in the future.

Agreed. We could have two tests. One with Stage(dvc_repo) to test that it works correctly when in the repo, and one more test with Stage(erepo.dvc) to test that it works when outside of the repo. Should be enough.

Suor · 2019-11-16T10:40:13Z

Considering all the test complications I suggest not touching existing ones, but adding a separate pytest-style test to check output stringification for an output in external repo.

pared · 2019-11-18T12:37:41Z

is this necessary?

If yes, then simply using as argument will do the job.

Looks like it's not necessary. Removing as erepo fixture already creates one.

pared · 2019-11-18T12:46:51Z

I think all those test could be done with one, where parametrize child, path and expected result. But I guess it is a matter of taste.

Parametrizing for one line test is just a boiler plate.

I tried that one but I found parameterizing expected_result as well too confusing for myself (and, too much repetition). What do you think?

@skshetry I am ok with this as it is

I would still prefer several asserts in a single test. This is mere indirection, no upsides.

I also prefer having multiple asserts, but I think, this is easier to follow and understand.

pared

One question, one note, besides that LGTM.

Suor · 2019-11-16T17:50:34Z

They do run, as the above TestCase is not changed at all. Or, did you mean this specific example?

Oh, I see, I was inattentive. And OutputLOCALBase not really used confused me.

You mean, for each remote separately? Please, clarify.

You don't need to do anything for each remote. You are only changing local one and thus only need to add tests for it. You can do that without touching existing tests at all and instead create two new test functions to check the specific changes you introduced.

Suor · 2019-11-18T13:25:39Z

I would call it path_isin() to be more in line with PathInfo.isin().

Renamed, thanks.

Suor · 2019-11-18T13:28:49Z

If yes, then simply using as argument will do the job.

Suor · 2019-11-18T13:33:02Z

Parametrizing for one line test is just a boiler plate.

Suor · 2019-11-18T13:36:28Z

What about is_child_path('a/b/', 'a/b')? Comparing only things built with os.path.join() doesn't quite handle it.

@Suor I think that os.path.join(path, "") which is inside is_child_path handles this one.

Yes, I forgot to test about the case (which fails atm). I'll add that (using os.path.join(), as I am not sure about testing via raw strings.)

From what I understood, you are asking me to also test strings directly? But, that'd not be platform agnostic. That specific string test might need skipping on Windows (to add to that, the implementation is already quite dependent on os.path.join() although it claims it's purely lexical :) ).

Also, should there be a test for demonstrating support for PathInfo? (saw #2137, up-level references might fail on PathInfo, maybe normpath could help.)

@Suor, I refactored for PathInfo, as well as added few tests for absolute path and the case you just mentioned.

skshetry · 2019-11-18T16:46:28Z

I have 12 commits already. I'd prefer rebasing/squashing (or, is it not preferred for some reasons?)

shcheklein · 2019-11-18T16:51:08Z

I have 12 commits already. I'd prefer rebasing/squashing (or, is it not preferred for some reasons?)

@pared @MrOutis is the team policy flexible now? @skshetry I think it's totally up to you to manage commits in your branch in any way! And it's of course better for us in terms of merging if they look nice and clean :)

Suor

Looks like we reached cleanup. So there are many comments below, but they should go easy.

Suor · 2019-11-18T16:49:06Z

We don't split imports like that anymore.

Suor · 2019-11-18T16:49:42Z

The last os.path.join() is not needed.

Suor · 2019-11-18T16:50:40Z

return child != parent and ...

Suor · 2019-11-18T16:54:41Z

I would still prefer several asserts in a single test. This is mere indirection, no upsides.

Suor · 2019-11-18T16:57:32Z

Why are using pass by name here? I suggest treating those args as positional only.

I thought, it'd be less confusing here if I added the keywords. Removing ...

Suor · 2019-11-18T16:58:26Z

Test is about substring not some completely different path, test name is inpresize.

Suor · 2019-11-18T17:15:34Z

I guess it won't work in Python 3.5 and earlier.

Oops, didn't test on Python3.5. Fixing ...

Suor · 2019-11-18T17:16:05Z

Move this to dvc.utils.fs please.

I thought, dvc.utils.fs were mostly related to low-level like inodes and stuffs. Moving ...

@Suor, I have moved it to dvc.utils.fs and the tests as well.

ghost

No additional comments 👍

skshetry · 2019-11-18T23:56:36Z

@Suor, I have rebased, please review again.

Suor · 2019-11-19T11:31:45Z

+    """Check if given `child` path is inside `parent`."""
+
+    def normalize_path(path):
+        return os.path.normpath(fspath(path))


It should be fspath_py35() instead of fspath() here, since in Python 3.6+ you may pass Path-like object to os utils directly.

Changed, thanks.

Suor · 2019-11-19T11:33:53Z

+
+def test_path_isin_on_same_path():
+    path = os.path.join("path", "to", "folder")
+    path2 = os.path.join(path, "")


Suggested change

path2 = os.path.join(path, "")

path_with_sep = os.path.join(path, "")

efiop · 2019-11-19T16:48:57Z

@skshetry Please check tests, they are failing.

efiop · 2019-11-19T16:49:41Z

Ah, wait, those failures are probably unrelated. Looking into it...

efiop · 2019-11-19T23:59:50Z

Fixed the tests on master, everything should be smooth now. Sorry for the inconvenience 🙂

pared · 2019-11-20T08:53:21Z

When this patch is merged, we should create ticket for replacing startswith with path_isin throughout the code.

efiop · 2019-11-20T09:03:40Z

@Suor @MrOutis Guys, please take a look.

Suor

Looks good

efiop · 2019-11-20T09:34:08Z

Ah, I see @MrOutis already approved.

efiop · 2019-11-20T09:34:27Z

Thanks @skshetry ! 🎉

efiop · 2019-11-20T09:57:44Z

@skshetry Please don't forget to create a ticket for #2798 (comment) . 🙂

shcheklein requested review from Suor and efiop November 16, 2019 02:26

skshetry force-pushed the fix-str-rel-repo branch from b23a015 to a836705 Compare November 16, 2019 02:43

skshetry commented Nov 16, 2019

View reviewed changes

Comment thread tests/unit/output/test_local.py Outdated

skshetry commented Nov 16, 2019

View reviewed changes

Comment thread tests/unit/output/test_local.py Outdated

skshetry commented Nov 16, 2019

View reviewed changes

Comment thread tests/unit/output/test_local.py Outdated

skshetry marked this pull request as ready for review November 16, 2019 07:52

skshetry commented Nov 16, 2019

View reviewed changes

Suor suggested changes Nov 16, 2019

View reviewed changes

weekly-digest Bot mentioned this pull request Nov 17, 2019

Weekly Digest (10 November, 2019 - 17 November, 2019) #2805

Closed

skshetry commented Nov 18, 2019

View reviewed changes

Comment thread tests/unit/utils/test_utils.py Outdated

skshetry commented Nov 18, 2019

View reviewed changes

Comment thread tests/unit/utils/test_utils.py Outdated

efiop requested review from a user, Suor and pared November 18, 2019 09:59

skshetry commented Nov 18, 2019

View reviewed changes

Comment thread tests/unit/utils/test_utils.py Outdated

pared reviewed Nov 18, 2019

View reviewed changes

Suor suggested changes Nov 18, 2019

View reviewed changes

ghost reviewed Nov 18, 2019

View reviewed changes

skshetry added 2 commits November 19, 2019 05:34

utils: implement path_isin to compare child and parent paths

68c7366

get/import: display path relative to temporary repo

f07a70a

skshetry force-pushed the fix-str-rel-repo branch from 704c420 to f07a70a Compare November 18, 2019 23:52

skshetry requested a review from Suor November 18, 2019 23:53

efiop requested a review from pared November 19, 2019 06:57

Suor suggested changes Nov 19, 2019

View reviewed changes

skshetry added 2 commits November 19, 2019 21:55

utils/fs: use fspath_py35 instead of fspath

58624e0

test: Refactor tests

83c8ab5

skshetry requested a review from Suor November 19, 2019 16:13

pared approved these changes Nov 20, 2019

View reviewed changes

efiop requested a review from a user November 20, 2019 09:03

Suor approved these changes Nov 20, 2019

View reviewed changes

efiop merged commit 91ac75d into treeverse:master Nov 20, 2019

skshetry deleted the fix-str-rel-repo branch November 20, 2019 09:47

skshetry mentioned this pull request Nov 20, 2019

Use path_isin instead of startswith to compare filepaths #2823

Closed

weekly-digest Bot mentioned this pull request Nov 24, 2019

Weekly Digest (17 November, 2019 - 24 November, 2019) #2841

Closed

	path2 = os.path.join(path, "")
	path_with_sep = os.path.join(path, "")

Conversation

skshetry commented Nov 16, 2019 • edited by jorgeorpinel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efiop commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Suor Nov 16, 2019 • 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!

skshetry Nov 16, 2019 • 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!

efiop Nov 18, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Suor Nov 16, 2019 • 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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

skshetry Nov 18, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pared left a comment

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!

Choose a reason for hiding this comment

Uh oh!

skshetry commented Nov 16, 2019 •

edited by jorgeorpinel

Loading

efiop commented Nov 16, 2019 •

edited

Loading

Suor Nov 16, 2019 •

edited

Loading

skshetry Nov 16, 2019 •

edited

Loading

efiop Nov 18, 2019 •

edited

Loading

Suor Nov 16, 2019 •

edited

Loading

skshetry Nov 18, 2019 •

edited

Loading

skshetry Nov 18, 2019 •

edited

Loading