Refactor pattern matching [cps/tasks/download.py: parse yt-dlp output for download progress percentage] #88

deldesir · 2024-01-10T19:16:05Z

These changes enhance the download task in several ways:

The download progress is now calculated dynamically based on the subprocess output
Updated the pattern to identify progress updates more reliably during the download process.
Cleaned up the code by removing redundant lines. Also, added the missing import statement for the 're' module.

Smoke and feature tested on Ubuntu 24.04

I reviewed the changes in the download task logic and subprocess handling.
I confirm that the dynamic progress calculation reflects the download progress.
I verified the improved subprocess output pattern for reliable progress tracking.
I checked the adjusted completion condition for 99% progress at 100% subprocess completion

Testing Steps in Calibre-Web UI

Initiate a download task for a YouTube url.
Monitor the progress updates in the tasks' view.
Verify that the progress is accurately calculated and reaches 100% upon successful completion.

Adjusted the download progress polling to dynamically capture progress updates from the subprocess output, enhancing accuracy.

Fix indentation

holta · 2024-01-10T20:12:21Z

cps/tasks/download.py

@deldesir Special Request:

Please communicate in 3-6 words the central purpose of each commit.

Not in the hidden secondary commit message. But in the actual commit message.

With non-descriptive commit messages like "Update download.py" it becomes unnecessarily complicated for everyone to understand (1) what is happening and (2) why. 😕

Goal: Commit Messages should Communicate Clear Intent! ✅

My bad, I thought I wrote "Fix indentation"

I see, I wrote it in description field. I thought I was putting the commit message in the title. It was an accident, sorry.

Everyone benefits when motivation is clear & concise e.g. anything like:

"cps/tasks/download.py: Fix indentation"

"download.py: Fix indenting in ~30 lines"

(Or anything similar!)

deldesir · 2024-01-10T20:34:47Z

cps/tasks/download.py

                            else:
-                                log.error("Failed to send the list of requested files to %s", self.original_url)
+                                self.progress = 0.99


FYI, I record the first 100% progress cycle as %99 so the script won't stop before yt-dlp finishes. There is often one or more quick 0%-100% cycles after the first one (for example the thumbnail and/or subtitles downloads)

I definitely don't understand it yet, ah well 😎

But thanks so much for explaining!

FYI, I record the first 100% progress cycle as %99 so the script won't stop before yt-dlp finishes. There is often one or more quick 0%-100% cycles after the first one (for example the thumbnail and/or subtitles downloads)

@deldesir ideally put some-or-all of this explanation in-line as a comment, near the Python code in question?

Please put this explanation (or similar) into a comment near the actual Python code.

Then let's move forward with merging this PR.

Thanks!

holta · 2024-01-10T20:46:41Z

Has this PR been quickly tested?

If so we should feel free to merge it, to keep community momentum!

deldesir · 2024-01-10T21:25:35Z

Yes, it is well tested.

Smoke and feature tested on Ubuntu 24.04

I reviewed the changes in the download task logic and subprocess handling.

I confirm that the dynamic progress calculation reflects the download progress.

I verified the improved subprocess output pattern for reliable progress tracking.

I checked the adjusted completion condition for 99% progress at 100% subprocess completion

Testing Steps in Calibre-Web UI

Initiate a download task for a YouTube url.

Monitor the progress updates in the tasks' view.

Verify that the progress is accurately calculated and reaches 100% upon successful completion.

deldesir · 2024-01-10T23:24:04Z

calibre-web/cps/tasks/download.py

Lines 47 to 57 in 4da5c46

    
           while p.poll() is None: 
        
               line = p.stdout.readline() 
        
               if line: 
        
                   if pattern_progress in line: 
        
                       percentage = int(re.search(r'\d+', line).group()) 
        
                       if percentage < 100: 
        
                           self.progress = percentage / 100 
        
                       else: 
        
                           self.progress = 0.99 
        
           p.wait()

The first 100% progress cycle is recorded as %99. This prevent the script from being stopped before yt-dlp finishes. There is often one or more quick 0%-100% cycles after the first one (for example the thumbnail and/or subtitles downloads)

holta · 2024-01-11T02:16:54Z

cps/tasks/download.py

@@ -41,44 +42,42 @@ def run(self, worker_thread):
                p = process_open(subprocess_args, newlines=True)

                # Define the pattern for the subprocess output
-                pattern_analyze = r"Running ANALYZE"
-                pattern_download = r"'action': 'download'"
+                pattern_progress = r"downloading"


The word "downloading" is an extremely common word, that might appear anywhere on a line in future.

Does the following make the regex a bit more precise, and future-proof?

Suggested change

pattern_progress = r"downloading"

# Equivalent Regex: https://github.com/iiab/calibre-web/blob/3c3c77f4dbf54ff093c3e3a68ac9b93a522c4070/scripts/lb-wrapper#L26

pattern_progress = r"^downloading"

Could use additional testing! Thanks @deldesir and @EMG70 if you can reconfirm Line 46 (now merged) is working well!

Bad News:

Regex "^downloading" was being used incorrectly yesterday. Functional testing didn't happen when it should have (to catch this serious oversight). ❌

Good News:

PR's like #90, #91 and #93 are getting much smaller and hence tangibly clearer ~ "TDD Craftsmanship" Organic + Incremental + Visible ~ nurturing a Responsive Culture of Collaboration.

By learning from our mistakes (the only way!) one day at a time 🌱 🌱 🌱

cps/tasks/download.py

Co-authored-by: A Holt <holta@users.noreply.github.com>

holta · 2024-01-11T03:30:49Z

Legibility = Understandability needs to be the central driving force going forward:

Sweeping indentation changes make PR's almost impossible to read ❌
Large-scale indentation changes need to be moved to a completely separate PR in future ✔️

Thank @deldesir for taking approachable (small) PR's very seriously!

AI² = Articulate Assumptions, Intent & Intuition

Co-authored-by: A Holt <holta@users.noreply.github.com>

holta · 2024-01-11T13:21:37Z

@deldesir any quick tip(s) you can provide to @EMG70 to help him reconfirm / validate this just-merged PR?

e.g. Is there anything specific he should look for in "Tasks" view today or in coming days?

calibre-web/cps/tasks/download.py

Lines 44 to 46 in d594aeb

    
           # Define the pattern for the subprocess output 
        
           # Equivalent Regex: https://github.com/iiab/calibre-web/blob/3c3c77f4dbf54ff093c3e3a68ac9b93a522c4070/scripts/lb-wrapper#L26 
        
           pattern_progress = r"^downloading"

deldesir · 2024-01-11T14:39:17Z

@deldesir any quick tip(s) you can provide to @EMG70 to help him reconfirm / validate this just-merged PR?

A positive scenario testing should finish with 100% recorded in tasks' progress column.

e.g. Is there anything specific he should look for in "Tasks" view today or in coming days?

We will work today on tasks' messages printed in Task column (this is a discussion for another PR)

Refactor pattern matching

42f99dd

Adjusted the download progress polling to dynamically capture progress updates from the subprocess output, enhancing accuracy.

deldesir requested a review from holta January 10, 2024 19:16

deldesir self-assigned this Jan 10, 2024

deldesir marked this pull request as draft January 10, 2024 19:16

Import missing re

07cc463

holta added the enhancement New feature or request label Jan 10, 2024

Update download.py

4da5c46

Fix indentation

deldesir marked this pull request as ready for review January 10, 2024 19:56

holta reviewed Jan 10, 2024

View reviewed changes

deldesir commented Jan 10, 2024

View reviewed changes

holta reviewed Jan 11, 2024

View reviewed changes

cps/tasks/download.py Show resolved Hide resolved

Update cps/tasks/download.py

a7815a9

Co-authored-by: A Holt <holta@users.noreply.github.com>

holta changed the title ~~Refactor pattern matching~~ Refactor pattern matching [cps/tasks/download.py: parse yt-dlp output for download progress percentage] Jan 11, 2024

Add percentage calculation comment in cps/tasks/download.py

99aeb47

Co-authored-by: A Holt <holta@users.noreply.github.com>

holta merged commit d594aeb into iiab:master Jan 11, 2024

holta mentioned this pull request Jan 11, 2024

Improve polling and logging #86

Closed

This was referenced Jan 11, 2024

Enhance download task messages for improved information when downloading a video #89

Closed

Update tasks messages based on the current status deldesir/calibre-web#20

Closed

Update tasks messages based on the current status #90

Merged

holta mentioned this pull request Jan 11, 2024

Revert to original pattern to match the word "downloading" #91

Closed

deldesir mentioned this pull request Jan 12, 2024

Redirect to STDOUT instead of STDERR: scripts/lb-wrapper #92

Closed

holta mentioned this pull request Jan 12, 2024

3 regex's (STDERR, STDIN, Python) to parse yt-dlp download progress, for live display in "Tasks" view #93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pattern matching [cps/tasks/download.py: parse yt-dlp output for download progress percentage] #88

Refactor pattern matching [cps/tasks/download.py: parse yt-dlp output for download progress percentage] #88

deldesir commented Jan 10, 2024 •

edited

Loading

holta Jan 10, 2024 •

edited

Loading

deldesir Jan 10, 2024

deldesir Jan 10, 2024 •

edited

Loading

holta Jan 10, 2024 •

edited

Loading

deldesir Jan 10, 2024

holta Jan 10, 2024

holta Jan 10, 2024

holta Jan 10, 2024

holta commented Jan 10, 2024

deldesir commented Jan 10, 2024

deldesir commented Jan 10, 2024 •

edited

Loading

holta Jan 11, 2024

deldesir Jan 11, 2024

holta Jan 11, 2024

holta Jan 12, 2024 •

edited

Loading

holta commented Jan 11, 2024 •

edited

Loading

holta commented Jan 11, 2024 •

edited

Loading

deldesir commented Jan 11, 2024

	pattern_progress = r"downloading"
	# Equivalent Regex: https://github.com/iiab/calibre-web/blob/3c3c77f4dbf54ff093c3e3a68ac9b93a522c4070/scripts/lb-wrapper#L26
	pattern_progress = r"^downloading"

Refactor pattern matching [cps/tasks/download.py: parse yt-dlp output for download progress percentage] #88

Refactor pattern matching [cps/tasks/download.py: parse yt-dlp output for download progress percentage] #88

Conversation

deldesir commented Jan 10, 2024 • edited Loading

holta Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deldesir Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

holta Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holta commented Jan 10, 2024

deldesir commented Jan 10, 2024

deldesir commented Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holta Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

holta commented Jan 11, 2024 • edited Loading

holta commented Jan 11, 2024 • edited Loading

deldesir commented Jan 11, 2024

deldesir commented Jan 10, 2024 •

edited

Loading

holta Jan 10, 2024 •

edited

Loading

deldesir Jan 10, 2024 •

edited

Loading

holta Jan 10, 2024 •

edited

Loading

deldesir commented Jan 10, 2024 •

edited

Loading

holta Jan 12, 2024 •

edited

Loading

holta commented Jan 11, 2024 •

edited

Loading

holta commented Jan 11, 2024 •

edited

Loading