-
Notifications
You must be signed in to change notification settings - Fork 158
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
whisperX to create transcripts #28
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WisperX implementation looks good to me! But I see some problems with the other files.
audio_builder.py
Outdated
@@ -26,6 +27,12 @@ | |||
cloudConfig = configparser.ConfigParser() | |||
cloudConfig.read('cloud_service_settings.ini') | |||
|
|||
# Get the video file name and create the output folder based on the original video file name | |||
originalVideoFile = os.path.abspath(batchConfig['SETTINGS']['original_video_file_path'].strip("\"")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some calls to the settings file with original_video_file_path
, however, the config file remains unchanged, I think that creating a commit with these new settings and a default value would avoid some unexpected behavior on users that do not look into the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to remove this for now
main.py
Outdated
@@ -72,6 +72,11 @@ | |||
originalVideoFile = os.path.abspath(batchConfig['SETTINGS']['original_video_file_path'].strip("\"")) | |||
srtFile = os.path.abspath(batchConfig['SETTINGS']['srt_file_path'].strip("\"")) | |||
|
|||
# Create the output folder based on the original video file name | |||
fileName = os.path.basename(originalVideoFile).split(".")[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same as the code in audio_builder.py
?
Also, there's already some code to handle the creation of output folders in Line 479 of the main file, in my opinion, moving this implementation there (or replacing the existing one) would be a better fit for this code.
Instead of adding this to requirements.txt, I'll probably want to implement it optionally somehow. Maybe add a message to be displayed when the script is run saying how to install it. |
PLEASE ONLY SUBMIT PULL REQUESTS FOR BUG FIXES OR LEGITIMATELY USEFUL ADDITIONAL FUNCTIONALITY
I know many of you are enthusiastic to help but I don't want this project to become bloated
Type of change
Proposed Changes
Additional Info
Checklist:
Required: