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

Improve polling and logging #86

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 51 additions & 37 deletions cps/tasks/download.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import os
import requests
import sqlite3
import subprocess
from datetime import datetime
from flask import flash
from flask_babel import lazy_gettext as N_, gettext as _

from cps.constants import SURVEY_DB_FILE
from cps.services.worker import CalibreTask, STAT_FINISH_SUCCESS, STAT_FAIL, STAT_STARTED, STAT_WAITING
from cps.subproc_wrapper import process_open
Expand All @@ -15,6 +15,7 @@
class TaskDownload(CalibreTask):
def __init__(self, task_message, media_url, original_url, current_user_name):
super(TaskDownload, self).__init__(task_message)
self.message = task_message
self.media_url = media_url
self.original_url = original_url
self.current_user_name = current_user_name
Expand All @@ -23,14 +24,14 @@ def __init__(self, task_message, media_url, original_url, current_user_name):
self.progress = 0

def run(self, worker_thread):
"""Run the download task"""
"""Run the download task."""
self.worker_thread = worker_thread
log.info("Starting download task for URL: %s", self.media_url)
self.start_time = self.end_time = datetime.now()
self.start_time = datetime.now()
self.stat = STAT_STARTED
self.progress = 0

lb_executable = self.get_lb_executable()
lb_executable = os.getenv("LB_WRAPPER", "lb-wrapper")

if self.media_url:
subprocess_args = [lb_executable, self.media_url]
Expand All @@ -39,53 +40,70 @@ def run(self, worker_thread):
# Execute the download process using process_open
try:
p = process_open(subprocess_args, newlines=True)

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

while p.poll() is None:
line = p.stdout.readline()
self.message = "Extracting metadata..."
if line:
log.info(line)
if pattern_analyze in line:
log.info("Matched output (ANALYZE): %s", line)
self.progress = 0.1
if pattern_download in line:
log.info("Matched output (download): %s", line)
self.progress = 0.5

if "creating" in line:
self.progress += 0.025
log.info("Download progress: %s", self.progress)
if "Running ANALYZE" in line:
self.progress = 0
line = p.stdout.readline()
if "extracting" in line:
self.message = "Downloading media..."
self.progress += 0.077
elif "downloading" in line:
self.progress += 0.077
elif "download" in line:
self.progress += 0.077
elif "merging" in line:
self.progress += 0.077
elif "adding" in line:
self.progress += 0.075
break
else:
self.progress += 0.077
log.info("Download progress: %s", self.progress)
Copy link
Member

@holta holta Jan 3, 2024

Choose a reason for hiding this comment

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

These lines need more explaining:

  • How were the 8 numbers chosen?
  • Why is 0.077 used 5 times, and 0.075 used once?
  • Why is a break statement now needed with "adding", unlike in the past?
  • Does 0.075 do anything at all, if it breaks out of the while loop, and is overwritten soon after (on Line 102) with self.progress = 1
  • Can the solitary test for "creating" (on Line 48) possibly be made a part of the if-elif-elif-elif-elif-else structure further below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored it and got rid of pattern matching. The logging will be more aligned with #68. However it builds on a customization made to xklb's underlying yt_dlp code, in which I use progress hooks.

Copy link
Member

Choose a reason for hiding this comment

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

builds on a customization made to xklb's underlying yt_dlp code

What xklb or yt-dlp code are you talking about, to help others understand?

in which I use progress hooks

What line number in your code? (Thanks!)

Copy link
Collaborator Author

@deldesir deldesir Jan 4, 2024

Choose a reason for hiding this comment

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

I am talking about this changes:
deldesir/library@f923477

Copy link
Member

@holta holta Jan 4, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I am talking about this changes: chapmanjacobd/[email protected]:library:main

@deldesir please clarify:

AI² = Articulate Assumptions, Intentions & Intuition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to customize xklb. IIAB install instructions remain the same.

p.wait()
# Database operations
try:
requested_files = []
with sqlite3.connect(SURVEY_DB_FILE) as conn:
requested_files = list(set([row[0] for row in conn.execute("SELECT path FROM media").fetchall() if not row[0].startswith("http")]))

# Database operations
requested_files = []
conn = sqlite3.connect(SURVEY_DB_FILE)
c = conn.cursor()
c.execute("SELECT path FROM media")
for row in c.fetchall():
requested_files.append(row[0])

c.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='playlists'")
if c.fetchone():
c.execute("SELECT title FROM playlists")
shelf_title = c.fetchone()[0]
else:
shelf_title = None
conn.close()
conn.execute("SELECT title FROM playlists")
shelf_title = conn.fetchone()[0] if conn.fetchone() else None

if self.original_url:
response = requests.get(self.original_url, params={"requested_files": requested_files, "current_user_name": self.current_user_name, "shelf_title": shelf_title})
if response.status_code == 200:
log.info("Successfully sent the list of requested files to %s", self.original_url)
else:
log.error("Failed to send the list of requested files to %s", self.original_url)
except sqlite3.Error as db_error:
if "no such table: playlists" in str(db_error):
log.info("No playlists table found in the database")
else:
log.error("An error occurred while trying to connect to the database: %s", db_error)
finally:
shelf_title = None
conn.close()

if self.original_url:
response = requests.get(self.original_url, params={"requested_files": requested_files, "current_user_name": self.current_user_name, "shelf_title": shelf_title})
if response.status_code == 200:
log.info("Successfully sent the list of requested files to %s", self.original_url)
else:
log.error("Failed to send the list of requested files to %s", self.original_url)

# Set the progress to 100% and the end time to the current time
self.progress = 1
self.end_time = datetime.now()
self.stat = STAT_FINISH_SUCCESS

except Exception as e:
except subprocess.CalledProcessError as e:
log.error("An error occurred during the subprocess execution: %s", e)
# Handling subprocess failure or errors
flash("Failed to complete the download process", category="error")
Expand All @@ -94,16 +112,12 @@ def run(self, worker_thread):
else:
log.info("No media URL provided")

def get_lb_executable(self):
lb_executable = os.getenv("LB_WRAPPER", "lb-wrapper")
return lb_executable

@property
def name(self):
return N_("Download")

def __str__(self):
return "Download %s" % self.media_url
return f"Download {self.media_url}"

@property
def is_cancellable(self):
Expand Down
2 changes: 1 addition & 1 deletion scripts/lb-wrapper
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ TMP_DOWNLOADS_DIR="/library/downloads/calibre-web"
SURVEY_DB_FILE="${TMP_DOWNLOADS_DIR}/survey.db"
URL="$1"
FORMAT_OPTIONS="--format best --format-sort 'tbr~1000'"
Copy link
Member

@holta holta Jan 9, 2024

Choose a reason for hiding this comment

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

Please delete scripts/lb-wrapper.greedy if you commit this suggestion: (as we discussed a few days ago)

Suggested change
FORMAT_OPTIONS="--format best --format-sort 'tbr~1000'"
# Or download largest possible HD-style / UltraHD videos, to try to force
# out-of-memory "502 Bad Gateway" for testing of issues like #37 and #79
# FORMAT_OPTIONS="--format-sort size"
FORMAT_OPTIONS="--format best --format-sort 'tbr~1000'"

Copy link
Member

@holta holta Jan 9, 2024

Choose a reason for hiding this comment

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

Please delete scripts/lb-wrapper.greedy if you commit this suggestion: (as we discussed a few days ago)

@deldesir see above request.

(This thread appears to have been closed prematurely.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll delete lb-wrapper.greedy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get it. I will delete lb-wrapper.greedy

XKLB_FULL_CMD="${XKLB_EXECUTABLE} tubeadd ${SURVEY_DB_FILE} ${URL} --verbose && ${XKLB_EXECUTABLE} dl ${SURVEY_DB_FILE} --prefix ${TMP_DOWNLOADS_DIR} --write-thumbnail ${FORMAT_OPTIONS} --video ${URL} --verbose"
XKLB_FULL_CMD="${XKLB_EXECUTABLE} tubeadd ${SURVEY_DB_FILE} ${URL} -vv && ${XKLB_EXECUTABLE} dl ${SURVEY_DB_FILE} --prefix ${TMP_DOWNLOADS_DIR} --write-thumbnail ${FORMAT_OPTIONS} --video ${URL} -vvv"

mkdir -p ${TMP_DOWNLOADS_DIR}

Expand Down
2 changes: 1 addition & 1 deletion scripts/lb-wrapper.greedy
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ TMP_DOWNLOADS_DIR="/library/downloads/calibre-web"
SURVEY_DB_FILE="${TMP_DOWNLOADS_DIR}/survey.db"
URL="$1"
FORMAT_OPTIONS="--format-sort size"
XKLB_FULL_CMD="${XKLB_EXECUTABLE} tubeadd ${SURVEY_DB_FILE} ${URL} --verbose && ${XKLB_EXECUTABLE} dl ${SURVEY_DB_FILE} --prefix ${TMP_DOWNLOADS_DIR} --write-thumbnail ${FORMAT_OPTIONS} --video ${URL} --verbose"
XKLB_FULL_CMD="${XKLB_EXECUTABLE} tubeadd ${SURVEY_DB_FILE} ${URL} -vv && ${XKLB_EXECUTABLE} dl ${SURVEY_DB_FILE} --prefix ${TMP_DOWNLOADS_DIR} --write-thumbnail ${FORMAT_OPTIONS} --video ${URL} -vvv"

mkdir -p ${TMP_DOWNLOADS_DIR}

Expand Down