fix get-url not fetching from url that does not support HEAD request by hl-a-k · Pull Request #4646 · treeverse/dvc

hl-a-k · 2020-09-30T07:35:13Z

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Fix #4251
The reason we do this save() call here is just to check if the source exists. So I replace this with
if in_repo: dep.save()
By the way, fixed a minor bug.
bug description:
1, environment: OS which default character is not 'UTF-8'. For example windows 10 which default character is 'gbk'.
2, operation:
python setup.py install
Traceback (most recent call last):
File "setup.py", line 149, in
long_description=open("README.rst", "r").read(),
UnicodeDecodeError: 'gbk' codec can't decode byte 0xa2 in position 41: illegal multibyte sequence

bug description: 1, environment: OS which default character is not 'UTF-8'. For example windows 10 which default character is 'gbk'. 2, operation: python setup.py install Traceback (most recent call last): File "setup.py", line 149, in <module> long_description=open("README.rst", "r").read(), UnicodeDecodeError: 'gbk' codec can't decode byte 0xa2 in position 41: illegal multibyte sequence

treeverse#4251

skshetry · 2020-09-30T10:56:57Z

Hi, @hl-a-k. Thanks for contributing. 🎉 🙂

It'd be helpful if you could provide more information regarding the PR/fix in the description above. Thanks, we'll review it soon.

efiop · 2020-10-03T00:20:05Z

+    if in_repo:
+        dep.save()


The unfortunate issue with not calling save is that our exceptions in the logic down below are not well unified, so it won't present a nice error when, for example, the source doesn't exist 🙁 I.e. download will trigger a method in one of the Tree classes (in dvc/tree/), and they all might throw a different backend-specific error, which is pretty ugly 🙁 We are on our way to unifying tree exceptions, but we are not there yet.

Might be missing something, but the only reason we do this save() call here is to check if the source exists. If we replace this with if not dep.exists(): raise DependencyDoesNotExistError(...), that should do the trick for now

Also, if I understand correctly, you've introduced this flag to avoid changing the API behavior, right? So like for a backward compatibility? If so, we could totally simply switch to the new behaviour and remove it.

You're right. By introducing this flag, we can keep the original logic unaffected.

Makes sense. So let's get rid of it and just replace

if in_repo: dep.save()

with

if not dep.exists: raise dep.DoesNotExistError(dep)

Changes committed. Seems nothing happened. ^_^

efiop · 2020-10-03T00:20:19Z

    version=version,
    description="Git for data scientists - manage your code and data together",
-    long_description=open("README.rst", "r").read(),
+    long_description=open("README.rst", "r", encoding="UTF-8").read(),


Oh, nice catch!

efiop · 2020-10-03T00:22:40Z

@hl-a-k Thank you for your patience and contribution 🙂 A few comments in addition to the ones above:

Looks like github is not able to associate your commits with your github profile. Please go to https://github.com/settings/emails and add the email used in your commits.
Please adjust the PR title to something more meaningful.
Please use Fix #issue-number (e.g. Fix get-url should just GET files #4251) in your PR description, so github will automatically close the issue when your PR is merged.

Thank you! 🙏

Btw, the reason we didn't reply earlier is that we were waiting for the PR description update and when you did edit it, github didn't send a notification (it doesn't for message edits, so that's expected). Please feel free to leave a comment in such cases next time, we'll be sure to get to it ASAP. 🙂

hl-a-k · 2020-10-03T02:40:38Z

@hl-a-k Thank you for your patience and contribution 🙂 A few comments in addition to the ones above:

Looks like github is not able to associate your commits with your github profile. Please go to https://github.com/settings/emails and add the email used in your commits.

Please adjust the PR title to something more meaningful.

Please use Fix #issue-number (e.g. Fix get-url should just GET files #4251) in your PR description, so github will automatically close the issue when your PR is merged.

Thank you! 🙏

Btw, the reason we didn't reply earlier is that we were waiting for the PR description update and when you did edit it, github didn't send a notification (it doesn't for message edits, so that's expected). Please feel free to leave a comment in such cases next time, we'll be sure to get to it ASAP. 🙂

hl-a-k · 2020-10-03T02:42:37Z

I have completed the changes.

efiop · 2020-10-03T02:52:50Z

@hl-a-k Looks like you didn't add zwh@geeboo.cn(used in your commits) to https://github.com/settings/emails . If you did, then maybe it is just github that needs some time to reparse this 🙂

This reverts commit f524c72

This reverts commit fa5db47

We can check if the source exists by using `dep.exists`

hl-a-k · 2020-10-03T06:00:15Z

@hl-a-k Looks like you didn't add zwh@geeboo.cn(used in your commits) to https://github.com/settings/emails . If you did, then maybe it is just github that needs some time to reparse this 🙂

Sorry, I don't use zwh@geeboo.cn anymore. I have updated my git configuration now.

efiop · 2020-10-03T17:46:45Z

    (dep,) = dependency.loads_from(None, [url])
    (out,) = output.loads_from(None, [out], use_cache=False)
-    dep.save()
+    if not dep.exists:


Sorry, I totally forgot that the issue itself was about exist not working for some urls 🙁 So this won't actually solve it. My mistake, sorry for the confusion.

Ok, let me see if the exception approach mentioned above is feasible to implement...

At least, this approach work for #4251 . I have tested.

How is the current situation？

Didn't get to research this yet. Need to confirm this and we'll merge if it is indeed fixed. Looks good as is. Thanks!

@hl-a-k Thanks for your patience! Indeed, it was the get_hash that was causing that issue, so the current solution works like charm. Thank you! 🙏

efiop

Thank you! 🚀

zwh added 3 commits September 30, 2020 15:14

bug treeverse#4251 fixed

fa5db47

treeverse#4251

bug treeverse#4251 fixed

f524c72

treeverse#4251

efiop added the awaiting response we are waiting for your reply, please respond! :) label Oct 3, 2020

efiop reviewed Oct 3, 2020

View reviewed changes

hl-a-k changed the title ~~#4251 bug fixed~~ Fixed the bug #4251 by a compatible way, and fixed a minor bug which causes of default character. Oct 3, 2020

hl-a-k changed the title ~~Fixed the bug #4251 by a compatible way, and fixed a minor bug which causes of default character.~~ Fixed the bug #4251 by a backward compatible way, and fixed a minor bug which causes of default character. Oct 3, 2020

hl-a-k changed the title ~~Fixed the bug #4251 by a backward compatible way, and fixed a minor bug which causes of default character.~~ Fixed the bug #4251 by a backward compatible way, and fixed a minor bug which causes of OS default character. Oct 3, 2020

hl-a-k added 3 commits October 3, 2020 12:37

Revert "bug treeverse#4251 fixed treeverse#4251"

28e99f8

This reverts commit f524c72

Revert "bug treeverse#4251 fixed treeverse#4251"

13a27a1

This reverts commit fa5db47

Fix bug treeverse#4251

b7203ed

We can check if the source exists by using `dep.exists`

hl-a-k changed the title ~~Fixed the bug #4251 by a backward compatible way, and fixed a minor bug which causes of OS default character.~~ Fixed the bug #4251, and fixed a minor bug which causes of OS default character. Oct 3, 2020

efiop reviewed Oct 3, 2020

View reviewed changes

efiop approved these changes Oct 12, 2020

View reviewed changes

efiop merged commit 59105ec into treeverse:master Oct 12, 2020

efiop added bugfix fixes bug and removed awaiting response we are waiting for your reply, please respond! :) labels Oct 12, 2020

skshetry changed the title ~~Fixed the bug #4251, and fixed a minor bug which causes of OS default character.~~ fix get-url not fetching from url that does not support HEAD request Oct 12, 2020

Conversation

hl-a-k commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

efiop Oct 3, 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!

efiop Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

hl-a-k commented Oct 3, 2020

Uh oh!

hl-a-k commented Oct 3, 2020

Uh oh!

efiop commented Oct 3, 2020

Uh oh!

hl-a-k commented Oct 3, 2020

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!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hl-a-k commented Sep 30, 2020 •

edited

Loading

skshetry commented Sep 30, 2020 •

edited

Loading

efiop Oct 3, 2020 •

edited

Loading

efiop Oct 3, 2020 •

edited

Loading

efiop commented Oct 3, 2020 •

edited

Loading