Skip to content
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

Adjust error message for stuck/slow videos [if elapsed_time >= timeout, e.g. due to unavailable fragments &/or xklb media_check] #223

Merged
merged 13 commits into from
Aug 17, 2024

Conversation

deldesir
Copy link
Collaborator

@deldesir deldesir commented Jul 15, 2024

This PR gets rid of the dozen multi-lines error messages

Tested on Ubuntu 24.04 (LRN2)
image

@holta
Copy link
Member

holta commented Jul 15, 2024

Lack of explanatory context is the far more serious problem:

  • Lack of link(s) to yt-dlp core issue(s) — if the problem is impossible to solve — link from "Tasks" view to a concrete explanation as to why.
  • Conversely, an actionable suggestion needs to be included in "Tasks" view — or a link to an actionable suggestion[*] — if users can take action.

(Either way: all users deserve a clear explanation of next steps!)

[*] Link to a concrete actionable steps at https://github.com/iiab/calibre-web/wiki if absolutely necessary.

Possibly related:

@deldesir
Copy link
Collaborator Author

deldesir commented Jul 15, 2024

Lack of explanatory context is the far more serious problem:

Is this ticket explanatory enough? yt-dlp/yt-dlp#2137

  • Lack of link(s) to yt-dlp core issue(s) — if the problem is impossible to solve — link from "Tasks" view to a concrete explanation as to why.

I'll put the following link in the error message yt-dlp/yt-dlp#2137

  • Conversely, an actionable suggestion needs to be included in "Tasks" view — or a link to an actionable suggestion[*] if users can take action.

What to suggest a user to do if nothing can be done. Redownloading/retrying is the only concrete action a user can do in such cases. There is no buttons or parameters to set in the GUI.

(Either way: all users deserve a clear explanation of next steps!)

[*] Link to a concrete actionable steps at https://github.com/iiab/calibre-web/wiki if absolutely necessary.

@deldesir deldesir marked this pull request as draft July 15, 2024 23:16
@@ -81,7 +81,7 @@ def run(self, worker_thread):
else:
elapsed_time = (datetime.now() - last_progress_time).total_seconds()
if elapsed_time >= fragment_stuck_timeout:
self.message += f"<br>Some fragments are taking longer than expected to download. Please wait..."
self.message = f"Unable to download {self.media_url_link} due to unavailable fragments. Please see https://github.com/yt-dlp/yt-dlp/issues/2137 for context. Try to download the video again later."
Copy link
Member

Choose a reason for hiding this comment

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

Try to download the video again later.

Specifically why? Under what conditions will waiting help?

Suggested change
self.message = f"Unable to download {self.media_url_link} due to unavailable fragments. Please see https://github.com/yt-dlp/yt-dlp/issues/2137 for context. Try to download the video again later."
self.message = f"Unable to download {self.media_url_link} due to unavailable fragments. See https://github.com/yt-dlp/yt-dlp/issues/2137 for context. Try to download the video again later."

Copy link
Member

@holta holta Jul 15, 2024

Choose a reason for hiding this comment

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

Try to download the video again later.

Specifically why? Under what conditions will waiting help?

Let's not create false hopes:

@holta
Copy link
Member

holta commented Jul 15, 2024

Is this ticket explanatory enough? yt-dlp/yt-dlp#2137

Can an anchor tag ( e.g. URL#anchor ) link to the most relevant message / context within yt-dlp/yt-dlp#2137 ?

@holta holta added documentation Improvements or additions to documentation enhancement New feature or request question Further information is requested labels Jul 16, 2024
@@ -81,7 +81,7 @@ def run(self, worker_thread):
else:
elapsed_time = (datetime.now() - last_progress_time).total_seconds()
if elapsed_time >= fragment_stuck_timeout:
self.message += f"<br>Some fragments are taking longer than expected to download. Please wait..."
self.message = f"Unable to download {self.media_url_link} due to unavailable fragments. Please see <a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>this ticket</a> for context. Try to download the video again later."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.message = f"Unable to download {self.media_url_link} due to unavailable fragments. Please see <a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>this ticket</a> for context. Try to download the video again later."
self.message = f"Unable to download {self.media_url_link} due to unavailable fragments. See <a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>yt-dlp/yt-dlp#2137</a> for context. Try to download the video again later (e.g. if [EXPLANATION])."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The user cannot do anything to work around this for now. I removed the suggestion to retry.

@holta
Copy link
Member

holta commented Jul 16, 2024

Is this ticket explanatory enough? yt-dlp/yt-dlp#2137

Can an anchor tag ( e.g. URL#anchor ) link to the most relevant message / context within yt-dlp/yt-dlp#2137 ?

  1. @deldesir if the explanation at the very top of --live-from-start didn't merge after live stream end. yt-dlp/yt-dlp#2137 is in fact the best explanation of the problem, then please confirm that an anchor tag is not useful in this particular case.
  2. If that's the case, can the title (subject line) of --live-from-start didn't merge after live stream end. yt-dlp/yt-dlp#2137 ("--live-from-start didn't merge after live stream end") be weaved into the "Tasks" view explanation to make it more accurate?
    1. Or if you believe the title (subject line) of --live-from-start didn't merge after live stream end. yt-dlp/yt-dlp#2137 is itself not quite accurate, please just explain what you think is really happening / what's the correct explanation, Thanks!

@holta
Copy link
Member

holta commented Jul 16, 2024

Excerpt from #215 (comment) :

Explain here why the error message is being repeated, so everyone can understand the root cause, and what it means.

Whereas suppressing error messages is almost always neither useful nor healthy.

@deldesir does repetition of this error message somehow indicate (or at least correlate?) with the severity of the problem with an individual video download?

(If so, should this [i.e. how much the error message is being repeated] somehow be communicated to the "Task" view user/operator?)

@deldesir
Copy link
Collaborator Author

deldesir commented Jul 16, 2024

the message was always repeated because it's caught within the loop of the polling mechanism looking for activity. This was triggered by inactivity for 30s since the video was stuck at x bytes.

elapsed_time = (datetime.now() - last_progress_time).total_seconds()
if elapsed_time >= fragment_stuck_timeout:
self.message += f"<br>Some fragments are taking longer than expected to download. Please wait..."

In this particular case, it was repeated with multiline because the error message was appended to previous self.message with self.message += "..."

@holta
Copy link
Member

holta commented Jul 16, 2024

Sounds like you're saying the error message should never really have been repeated (using += appending) in the first place — if the repetition was completely meaningless? (In other words in PR #211 about 2 weeks ago.)

(No problem, but if so, forgive me for assuming that repetition had been added for a reason!)

@deldesir
Copy link
Collaborator Author

deldesir commented Jul 16, 2024

Sounds like you're saying the error message should never really have been repeated (using += appending) in the first place — if the repetition was completely meaningless? (In other words in PR #211 about 2 weeks ago.)

That's right

(No problem, but if so, forgive me for assuming that repetition had been added for a reason!)

No worry, sometimes I cannot explain stuff well enough. The self.message is constantly being replaced by the same message when there is no activity for 30s. the += made it multi-line. This was a mistake.

@holta
Copy link
Member

holta commented Jul 16, 2024

No worries!!

Let's just make the new error message as accurate as possible, perhaps including bits of the yt-dlp/yt-dlp#2137 title / subject line if this explains more clearly + completely, per: #223 (comment)

@holta
Copy link
Member

holta commented Jul 17, 2024

@deldesir please explain why this appears to be happening — hinting at your best guess {assumptions, intuition, intention} :

  1. Explain how yt-dlp flag --live-from-start(--live-from-start didn't merge after live stream end. yt-dlp/yt-dlp#2137) can sometimes (often?!) be related?

And/Or...

  1. Summarize the situation — also linking to How to handle missing/incomplete fragments more elegantly yt-dlp/yt-dlp#6078 — if that adds useful context re: unavailable fragment failures, e.g. for Formerly live https://youtu.be/4BL65HElOPg fails to download [same "no such column: error" issue as #186 ?] [unavailable fragments?] #215 ?

And/Or...

  1. Mention "fragments failing to merge" (and what that means?!) if that discussion on --live-from-start didn't merge after live stream end. yt-dlp/yt-dlp#2137 adds critical understanding... ?

🙏

@deldesir
Copy link
Collaborator Author

deldesir commented Jul 19, 2024

@holta, this is difficult to put an accurate message for this bug. My observation is we never know if the stuck video will fail or recover.

For example, the following video was stuck, but ended up being donwloaded succesfully. I'd like to put a message to encourage the user to wait, but we don't know if it's worth it.

image

after 2-3 minutes:

image

UPDATE: This specific video was not stuck. It was reported as such because the post-processing took some time. I ran lb-wrapper dl https://www.youtube.com/watch?v=ZdzGp2eWM5o and could see the progress reached 100%.

image

We don't know wether the download will recover or not
@holta
Copy link
Member

holta commented Jul 19, 2024

UPDATE: This specific video was not stuck. It was reported as such because the post-processing took some time.

Great insight! ✅

Hopefully "Tasks" view will clarify this & similar revelations as soon as you can in coming days — one tiny-or-small PR at a time — as soon as each is safe and reasonable to merge! 🪚

@holta holta changed the title Adjust error message for stuck videos Adjust error message for stuck videos [if elapsed_time >= fragment_stuck_timeout] Jul 31, 2024
@holta
Copy link
Member

holta commented Jul 31, 2024

I suggest we go with a better explanation while I am working on the cause of the video being stuck.

timeout mechanism was designed specifically for missing fragments issues.

@deldesir are you suggesting that error message "stuck due to unavailable fragments" is sometimes incorrect?

  • If yes, should error message say "stuck, likely due to unavailable fragments" ? (Or "stuck, possibly due to unavailable fragments" ?)
  • If no, what kind of a "better explanation" is appropriate?

Thank you for clearing this up, so this PR (or similar) can be merged without delay. (As mentioned a week ago, this can always be revised later if understanding advances + shifts in due course!)

@deldesir
Copy link
Collaborator Author

@deldesir are you suggesting that error message "stuck due to unavailable fragments" is sometimes incorrect?

Yes, tasks may appear stuck due to delays in processing video files by ffmpeg too.

A message like "Processing video xxx is taking longer than usual, please wait..." will do. If for some reason the download task fails, we can let the user know about it with another message like: "Video xxx failed to download due to xxx"

@holta
Copy link
Member

holta commented Aug 13, 2024

tasks may appear stuck due to delays in processing video files by ffmpeg too.

If there are delays or failures implementing FFmpeg in isolated circumstance — e.g. if "instantly" generating missing thumbnails is necessary for some video downloads — we'd surface those delays or failures (FFmpeg or otherwise!) making them increasingly very visible in Tasks view. Related:

A message like "Processing video xxx is taking longer than usual, please wait..." will do.

Please include a link that (also) explains real world causes of delay:

@holta
Copy link
Member

holta commented Aug 13, 2024

If there are delays or failures implementing FFmpeg in isolated circumstance — e.g. if "instantly" generating missing thumbnails is necessary for some video downloads — we need to surface those delays or failures (FFmpeg or otherwise!) making them increasingly very visible in Tasks view.

Let's outline in a separate GitHub issue or PR the most practical way(s) to surface + strengthen clarity (concrete explanation, hints, tips, or context!) in Tasks view.

Addressing similar/common errors, cleanly + efficiently in "Tasks" view, over coming weeks. 💫

Related:

@holta
Copy link
Member

holta commented Aug 13, 2024

@deldesir linking to this actual ticket (#223) is also fine in the interim, if you prefer?

@holta
Copy link
Member

holta commented Aug 13, 2024

A message like "Processing video xxx is taking longer than usual, please wait..." will do.

Given the reality that this error is triggered if elapsed_time >= fragment_stuck_timeout :

  • Can we be more transparent & forthright, telling users exactly that (in Tasks view) i.e. allowing them to see what's really happening❓

  • Or if fragment_stuck_timeout needs to be renamed, or slightly re-engineered, that is fine too ✅

@holta
Copy link
Member

holta commented Aug 15, 2024

"Processing video xxx is taking longer than usual, please wait..."

"Video xxx failed to download due to xxx"

Recap:

  1. Let's be Precise: "processing" is not the same as "downloading". 64c0b55 (July 19) mentions "Make message less categoric / We don't know whether the download will recover or not" — suggesting that "processing" might not be accurate❓

  2. Definitely mention likely reasons for delay (e.g. fragment download &/or FFmpeg thumbnail creation &/or xklb's media_check) in the actual error message, nurturing continuous progress 🔎

  3. Every error message should include a link to a relevant GitHub ticket comment (or https://github.com/iiab/calibre-web/wiki section, or whatever link is most meaningful!) to cultivate participACTION 🪜

@holta
Copy link
Member

holta commented Aug 15, 2024

ASIDE: Should we preserve the original += to repeat the error message in "Tasks" view every 2 minutes (120 seconds instead of 30 seconds as a result of PRs #227 and #229) since there are now 4X fewer error messages?

  • The argument in favor of += would be:
    Users expect and deserve to see regular progress indicators / explanation for what is happening! Reiterating the error message every 2 min might be appreciated by some, as a stronger visual indicator, if it doesn't create too much clutter? 🔢
  • The argument against += would be:
    Users can already watch the time counter increment every few seconds (or freeze up!) during delays — e.g. some might feel that reiterating the same error message every 2 min doesn't help? ⏳

We renamed the variable from "fragment_stuck_timeout" --> "timeout" to be more generic. We don't know if we are in the download or the processing state for the video.
@deldesir
Copy link
Collaborator Author

ASIDE: Should we preserve the original += to repeat the error message in "Tasks" view every 2 minutes (120 seconds instead of 30 seconds as a result of PRs #227 and #229) since there are now 4X fewer error messages?

  • The argument in favor of += would be:
    Users expect and deserve to see regular progress indicators / explanation for what is happening! Reiterating the error message every 2 min might be appreciated by some, as a stronger visual indicator, if it doesn't create too much clutter? 🔢
  • The argument against += would be:
    Users can already watch the time counter increment every few seconds (or freeze up!) during delays — e.g. some might feel that reiterating the same error message every 2 min doesn't help? ⏳

I think it's a bad idea. Multiline messages convey progress, but spamming the message column with repetitive messages is very ugly.

Copy link
Member

@avni avni left a comment

Choose a reason for hiding this comment

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

Line 84: As discussed, here's an alternative message to make clear the possible types of errors based on your explanation to me. I also added a sentence so the user knows we are retrying and should wait.

===
{self.media_url_link} is taking longer than expected. It could be a stuck download due to unavailable fragments (yt-dlp/yt-dlp#2137) and/or an error in xklb’s media check. Please wait as we try again. See <a href=‘#223' target=‘_blank’>/pull/223 for more info.

self.message += f"<br>Some fragments are taking longer than expected to download. Please wait..."

if elapsed_time >= timeout:
self.message = f"{self.media_url_link} is stuck due to unavailable fragments. See <a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>yt-dlp/yt-dlp#2137</a> for context."
Copy link
Member

@avni avni Aug 16, 2024

Choose a reason for hiding this comment

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

Suggested change
self.message = f"{self.media_url_link} is stuck due to unavailable fragments. See <a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>yt-dlp/yt-dlp#2137</a> for context."
self.message = f"{self.media_url_link} is taking longer than expected. It could be a stuck download due to unavailable fragments (https://github.com/yt-dlp/yt-dlp/issues/2137) and/or an error in xklb’s media check. Please wait as we try again. See <a href=https://github.com/iiab/calibre-web/pull/223' target=_blank’>https://github.com/iiab/calibre-web/pull/223 for more info.</a>"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is better.

@holta
Copy link
Member

holta commented Aug 16, 2024

Description looks great!

Both HTML links appear slightly off to me??

@deldesir can you abbreviate the visible part of links... to be?

  • yt-dlp/yt-dlp#2137
  • #223

@avni
Copy link
Member

avni commented Aug 17, 2024

Tested locally with Blondel via screen share on an IIAB instance. This PR is Ready to merge, imo.

[NB: http://lrn2.org's IP is blocked from downloading videos so we could not test there! See: yt-dlp/yt-dlp/issues/10128]

Screenshot 2024-08-16 at 8 11 22 PM

@holta holta changed the title Adjust error message for stuck videos [if elapsed_time >= fragment_stuck_timeout] Adjust error message for stuck/slow videos [if elapsed_time >= timeout, e.g. due to unavailable fragments &/or xklb media_check] Aug 17, 2024
self.message += f"<br>Some fragments are taking longer than expected to download. Please wait..."

if elapsed_time >= timeout:
self.message = f"{self.media_url_link} is taking longer than expected. It could be a stuck download due to unavailable fragments (<a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>yt-dlp/yt-dlp#2137</a>) and/or an error in xklb's media check. Please wait as we try again. See <a href='https://github.com/iiab/calibre-web/pull/223' target='_blank'>#223</a> for more info."
Copy link
Member

Choose a reason for hiding this comment

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

@deldesir @avni is the revision below more accurate? (Or if I'm wrong, LMK!)

Suggested change
self.message = f"{self.media_url_link} is taking longer than expected. It could be a stuck download due to unavailable fragments (<a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>yt-dlp/yt-dlp#2137</a>) and/or an error in xklb's media check. Please wait as we try again. See <a href='https://github.com/iiab/calibre-web/pull/223' target='_blank'>#223</a> for more info."
self.message = f"{self.media_url_link} is taking longer than expected. It could be a stuck download due to unavailable fragments (<a href='https://github.com/yt-dlp/yt-dlp/issues/2137' target='_blank'>yt-dlp/yt-dlp#2137</a>) and/or an error in xklb's media_check. Please wait as we keep trying. See <a href='https://github.com/iiab/calibre-web/pull/223' target='_blank'>#223</a> for more info."

Copy link
Member

@avni avni Aug 17, 2024

Choose a reason for hiding this comment

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

"keep trying" is more accurate, yes! TY! I don't know if 'media_check' is more accurate than 'media check'. @deldesir is more familiar with xklb.

Copy link
Member

Choose a reason for hiding this comment

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

@deldesir please revise at any time (e.g. with a new PR) if improvement's helpful 💯

Copy link
Member

Choose a reason for hiding this comment

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

Merging of this PR is now cleaned up with this separate commit:

(Apologies I messed up, and didn't get that revision from ~30min ago properly into this PR!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants