Enhance timeout check to avoid false positives during processing [of video download to IIAB] #170
Conversation
deldesir
changed the title
Also make code more concise and reliable
This goes on par with xklb execution. Fails only when xklb fails
Awesome news: Did a couple LRN2 tests (Ubuntu 24.04) confirm? |
|
I made more than a couple tests on t4. I'll do playlist test right away on lrn2. |
Single videos and playlists now work.
|
cps/tasks/download.py
Outdated
| from ..constants import XKLB_DB_FILE | ||
| from ..services.worker import CalibreTask, STAT_FINISH_SUCCESS, STAT_FAIL, STAT_STARTED, STAT_WAITING | ||
| from ..subproc_wrapper import process_open |
There was a problem hiding this comment.
Absolute imports are usually preferred because relative imports (..) can lead to confusion in module resolution.
Does an absolute path starting from the package name work?
| from ..constants import XKLB_DB_FILE | |
| from ..services.worker import CalibreTask, STAT_FINISH_SUCCESS, STAT_FAIL, STAT_STARTED, STAT_WAITING | |
| from ..subproc_wrapper import process_open | |
| from calibreweb.cps.constants import XKLB_DB_FILE | |
| from calibreweb.cps.services.worker import CalibreTask, STAT_FINISH_SUCCESS, STAT_FAIL, STAT_STARTED, STAT_WAITING | |
| from calibreweb.cps.subproc_wrapper import process_open |
There was a problem hiding this comment.
Absolute paths work. I was not comfortable seeing them flagged as unresolved in vscode. Thanks.
There was a problem hiding this comment.
I had to remove the package name from absolute imports. It will work starting with cps.
Co-authored-by: Bernie Innocenti <[email protected]>
cps/tasks/download.py
Outdated
| @@ -132,7 +129,7 @@ def run(self, worker_thread): | |||
| self.stat = STAT_FINISH_SUCCESS | |||
| log.info("Download task for %s completed successfully", self.media_url) | |||
| else: | |||
| self.end_time = datetime.now() | |||
| self.end_time = datetime.now() | |||
There was a problem hiding this comment.
If end_time needs to be set on both branches (lines 128 and 132), do it just once above the if.
By the way, this class sets end_time in multiple places (e.g. line 26 and 72). If the goal is to record the end time after the task completes, can these lines be eliminated?
Note: in case end_time ends up being unset, CalibreTask.runtime() will fail. The finally should catch all exit paths from the while loop, except when self.media_url is unset (which skips the entire thing).
There was a problem hiding this comment.
The goal is to record the run time dynamically per
https://github.com/iiab/calibre-web/blob/eb8302387b75714c9ab8addafac5df8af74e93c8/cps/services/worker.py#L232C5-L235C1
I'll remove the redundancy for 128 and 132, but if I remove 26 and 72, I will lose the dynamic run time messages. I'll move 72 to be more efficient.
|
Safe to merge? |
Yes |
holta
changed the title
This PR fixes the very frustrating timeout issue which led to premature moves of video files. Now the download progress polling goes on par with xklb's
lb dlexecution. Upon [reaching] the timeout limit, the user will only be flashed a status message about the [abnormal] delay. The task will fail due to unavailable fragment only when xklb fails.Tested rigorously and proudly on Ubuntu 24.10 (t4). Fix applied successfully on LRN2.