Modernize codebase: auto-detect GPU/CPU, replace print with logging, update deps - #78
Conversation
There was a problem hiding this comment.
Code Review
This pull request modernizes the repository by updating to Python 3, replacing print statements with a structured logging configuration, and transitioning from requirements.txt to Conda environment files. Key improvements include updating ffmpeg subprocess calls for better error handling, removing deprecated torch.autograd.Variable usage, and refactoring the scenedetect integration to support newer API versions. Review feedback highlights a potential TypeError when logging PyTorch tensors without converting them to scalars and identifies compatibility issues with weights_only=True in torch.load when handling full model pickles.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modernizes the repository by migrating to Python 3, replacing print statements with the logging module, and introducing conda environment files for dependency management. Significant changes include updating PySceneDetect API usage, improving FFmpeg command execution via subprocess.run, and enhancing security by using weights_only=True in torch.load. Review feedback identifies a missed logger conversion in SyncNetModel.py and a redundant file-opening operation in run_pipeline.py.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by upgrading to Python 3, introducing structured logging, and improving environment management via Conda. Key technical improvements include the removal of deprecated PyTorch features like Variable, the use of weights_only=True for secure model loading, and the transition to subprocess.run for more robust external command execution. The review feedback highlights two important reliability issues: the need for math.isclose when comparing floating-point durations to avoid precision errors, and the requirement to explicitly release video resources in run_pipeline.py to prevent potential file handle leaks.
Summary
SyncNetInstanceandS3FDnow auto-detect CUDA availability and fall back to CPU. All.cuda()calls replaced with.to(device).print()calls with Pythonloggingmodule across every script.subprocess.call(str, shell=True)withsubprocess.run(list)to prevent command injection. Replacedpdb.set_trace()error handling with proper exceptions.torch.autograd.Variable, addedweights_only=Truetotorch.load(), fixednp.int→np.intp, modernized NMS tensor operations in S3FD to remove deprecatedout=resize pattern.scenedetect==0.5.1to0.6.7.1, rewrotescene_detect()to use the modernopen_video/SceneManagerAPI.super()without args, removed trailing semicolons,#!/usr/bin/env python3shebangs,mkdir -pin download script, removed unused imports,os.makedirs(exist_ok=True).Test plan
sh download_model.sh— verify model and weights download without errorspython demo_syncnet.py --videofile data/example.avi --tmp_dir data/work/pytmp— verify AV offset ~3, confidence ~10run_pipeline.py→run_syncnet.py→run_visualise.py) on a test videoCUDA_VISIBLE_DEVICES="") to verify fallback works