-
Notifications
You must be signed in to change notification settings - Fork 28
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
ENH: log dandischema version and ensure we log (consistently) path for log messages in download #1499
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1499 +/- ##
==========================================
+ Coverage 88.56% 88.65% +0.08%
==========================================
Files 78 78
Lines 10585 10585
==========================================
+ Hits 9375 9384 +9
+ Misses 1210 1201 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dandi/download.py
Outdated
else: | ||
lgr.debug( | ||
"Download directory found, but digests do not match; starting new download" | ||
"%s - download directory found, but digests do not match;" | ||
" starting new download", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would digests.keys() and self. digests.keys() be useful to know here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt like I wondered the same and then dismissed as it would be "too much noise". but now not sure -- it might be... yet to decide
dandi/download.py
Outdated
@@ -853,12 +869,35 @@ def __exit__( | |||
exc_tb: TracebackType | None, | |||
) -> None: | |||
assert self.fp is not None | |||
if exc_type is not None or exc_val is not None or exc_tb is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exc_tb
is not used in the log here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first I had it but then kicked out from actual log -- the point is that if any of them provided, exception happened.
…r log messages in download
This grew too big and unwildly. I will separate latter commits into a separate PR or PRs. |
Current desire/need is to troubleshoot #1499 but moreover -- Windows. Not yet even sure if would work for windows but still -- would be good to have generally May be later we should parametrize more, not for every debug run
I shrunk to 1 commit, if passes, would merge |
3.8 windows:
anyways -- did not spot related fails, let's proceed |
🚀 PR was released in |
hopefully would be in assist to troubleshoot issues with downloads, like the one now reported by @Kevancic on slack