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

Suggestion: Get rid of running python scripts in shell #49

Open
alexlnkp opened this issue Jun 13, 2024 · 4 comments
Open

Suggestion: Get rid of running python scripts in shell #49

alexlnkp opened this issue Jun 13, 2024 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@alexlnkp
Copy link
Contributor

The title might be a bit confusing, but what i'm referring to is how some python scripts are ran in a subshell, even though they could be used as regular python modules.

cmd = (
    '"%s" infer/modules/train/extract/extract_f0_print.py "%s/logs/%s" %s %s'
    % (
        config.python_cmd,
        now_dir,
        exp_dir,
        n_p,
        f0method,
    )
)
logger.info("Execute: " + cmd)
p = Popen(
    cmd, shell=True, cwd=now_dir
)  # , stdin=PIPE, stdout=PIPE,stderr=PIPE

Is something that is present A LOT in the web.py file, but i fail to understand why even do that if you can create a job in the python that executes code, instead of opening a shell as a subprocess.

I assume this will be fixed when the RVC becomes a module, however I'll prefer fixing this sooner than later.

Opening a subshell and executing a python script adds a huge amount of overhead and introduces extreme amounts of security vulnerabilities, shell=True is one of the culprits for that in particular.

Also, the code will look just more appealing without all of the subprocess creation within shells and all the yields of the stdout as a generator for gradio interface to update properly

@fumiama
Copy link
Owner

fumiama commented Jun 14, 2024

but i fail to understand why even do that if you can create a job in the python that executes code

The initial author do not know how to do that. He just want a subprocess to run some code asynchronously.

@fumiama
Copy link
Owner

fumiama commented Jun 14, 2024

Also, the code will look just more appealing without all of the subprocess creation

Yes. And it will make it possible to add a button break in gradio.

@alexlnkp
Copy link
Contributor Author

Also, the code will look just more appealing without all of the subprocess creation

Yes. And it will make it possible to add a button break in gradio.

That reminds me of my Mangio-RVC-Fork and how painful it was to add the stop training button 😂

@fumiama
Copy link
Owner

fumiama commented Jun 14, 2024

how painful it was to add the stop training button

Yes 😂. So very important to change the code structure. It is the base to the further improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants