mrq/ai-voice-cloning
31
74
Fork 43
Code Issues 222 Pull Requests 5 Packages Projects Releases Wiki Activity

favor existing arguments from parameters (kwargs) over global (args) #336

Merged
mrq merged 1 commits from ben_mkiv/ai-voice-cloning:master into master 2023-08-23 21:05:39 +00:00
Conversation 3 Commits 1 Files Changed 1 +16 -2
ben_mkiv commented 2023-08-22 21:41:43 +00:00
Contributor
Copy Link

Hi last one for today...
which you should please review as i'm not sure if this could lead to unintended behavior. (i've tested generation in web interface, cli and with websockets to ensure that it still behaves as expected)

made this change because i couldn't find another way to force load a model specified from my websocket requests (which are passed into generate as kwargs)

if possible, could you give a short explanation of args? had a hard time twisting my head around the parameter parsing...
is this some global settings which get deduced from ai-voice-cloning defaults and whatever the user has configured in the webui settings?

Hi last one for today... which you should please review as i'm not sure if this could lead to unintended behavior. (i've tested generation in web interface, cli and with websockets to ensure that it still behaves as expected) made this change because i couldn't find another way to force load a model specified from my websocket requests (which are passed into generate as kwargs) if possible, could you give a short explanation of args? had a hard time twisting my head around the parameter parsing... is this some global settings which get deduced from ai-voice-cloning defaults and whatever the user has configured in the webui settings?
ben_mkiv added 2 commits 2023-08-22 21:41:52 +00:00 
removed load_tts() call, the cli tool now relys on generate() to load the tts system, which is less fail prone imho
mrq commented 2023-08-23 15:12:59 +00:00
Owner
Copy Link

which you should please review as i'm not sure if this could lead to unintended behavior. (i've tested generation in web interface, cli and with websockets to ensure that it still behaves as expected)

It looks fine, but I've been a bit out of my element for a while in terms of AIVC, so.

If you tested it and it works fine still, it should be fine.

made this change because i couldn't find another way to force load a model specified from my websocket requests (which are passed into generate as kwargs)

That argument is already grabbed under ./src/utils.py and referenced as args.autoregressive_model.

if possible, could you give a short explanation of args? had a hard time twisting my head around the parameter parsing...
is this some global settings which get deduced from ai-voice-cloning defaults and whatever the user has configured in the webui settings?

Yes. args will load defaults from the ./config/exec.json, and any passed in arguments are applied on top of it (without saving).

The arguments under ./src/cli.py extend those arguments with generation parameters (and those defaults are pulled from ./config/generate.json) as a bandaid, because desu this entire repo is a mess of stapling spaghetti on top of one another and is in dire need of a rewrite.

(tho it still loads the default model first, and then loads the target model, which seems to be because TTS loading just loads whatever is set in the settings first)

This is because, if you're relying on the generate function handling model loading, the web UI will load the TTS backend with whatever's specified under args.autoregressive_model first naively, and then look at whatever generation settings require it.

I would need to have args.autoregressive_model updated before the load_tts call.

> which you should please review as i'm not sure if this could lead to unintended behavior. (i've tested generation in web interface, cli and with websockets to ensure that it still behaves as expected) It looks fine, but I've been a bit out of my element for a while in terms of AIVC, so. If you tested it and it works fine still, it should be fine. > made this change because i couldn't find another way to force load a model specified from my websocket requests (which are passed into generate as kwargs) That argument is already grabbed under [`./src/utils.py`](https://git.ecker.tech/mrq/ai-voice-cloning/src/branch/master/src/utils.py#L3330) and referenced as `args.autoregressive_model`. > if possible, could you give a short explanation of args? had a hard time twisting my head around the parameter parsing... is this some global settings which get deduced from ai-voice-cloning defaults and whatever the user has configured in the webui settings? Yes. `args` will load defaults from the `./config/exec.json`, and any passed in arguments are applied on top of it (without saving). The arguments under `./src/cli.py` extend those arguments with generation parameters (and those defaults are pulled from `./config/generate.json`) as a bandaid, because desu this entire repo is a mess of stapling spaghetti on top of one another and is in dire need of a rewrite. > (tho it still loads the default model first, and then loads the target model, which seems to be because TTS loading just loads whatever is set in the settings first) This is because, if you're relying on the generate function handling model loading, the web UI will load the TTS backend with whatever's specified under `args.autoregressive_model` first naively, and then look at whatever generation settings require it. I would need to have `args.autoregressive_model` updated before the `load_tts` call.
ben_mkiv added 1 commit 2023-08-23 17:11:55 +00:00 
This reverts commit d1dbe3e464.
ben_mkiv added 1 commit 2023-08-23 17:21:56 +00:00 
This reverts commit d1dbe3e464.
ben_mkiv added 1 commit 2023-08-23 17:23:36 +00:00 
This reverts commit 89102347a9.

args are now updated in the websocket server
ben_mkiv added 1 commit 2023-08-23 17:24:46 +00:00
ben_mkiv added 2 commits 2023-08-23 17:29:42 +00:00 
favor existing arguments from parameters (kwargs) over global (args)

added parameter to specify the autoregressive_model (tho it still loads the default model first, and then loads the target model, which seems to be because TTS loading just loads whatever is set in the settings first)
removed load_tts() call, the cli tool now relys on generate() to load the tts system, which is less fail prone imho

Revert "added parameter to specify the autoregressive_model (tho it still loads the default model first, and then loads the target model, which seems to be because TTS loading just loads whatever is set in the settings first)"

This reverts commit d1dbe3e464.

Revert "added parameter to specify the autoregressive_model (tho it still loads the default model first, and then loads the target model, which seems to be because TTS loading just loads whatever is set in the settings first)"

This reverts commit d1dbe3e464.

Revert "favor existing arguments from parameters (kwargs) over global (args)"

This reverts commit 89102347a9.

args are now updated in the websocket server

remove unused import
ben_mkiv force-pushed master from 3ad0685620 to ce24ba41e2 2023-08-23 17:41:22 +00:00 Compare
ben_mkiv commented 2023-08-23 17:42:42 +00:00
Author
Contributor
Copy Link

thank you, i've reduced the PR to overriding the args parameters in the websocket server, ignore the commit spam... squashed it down to one commit

thank you, i've reduced the PR to overriding the args parameters in the websocket server, ignore the commit spam... squashed it down to one commit
mrq merged commit e613299304 into master 2023-08-23 21:05:39 +00:00
mrq commented 2023-08-23 21:06:48 +00:00
Owner
Copy Link

ignore the commit spam... squashed it down to one commit

No worries.

> ignore the commit spam... squashed it down to one commit No worries.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
insufficient info

Need more information

  
invalid

Something is wrong

  
news

To make up for a lack of a way to provide any news-like

  
not a bug

For non-bug things

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
bug
duplicate
enhancement
help wanted
insufficient info
invalid
news
not a bug
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: mrq/ai-voice-cloning#336
No description provided.
Delete Branch "ben_mkiv/ai-voice-cloning:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?