-
Notifications
You must be signed in to change notification settings - Fork 570
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
subprocess.run -> run_subprocess #352
Conversation
src/huggingface_hub/repository.py
Outdated
@@ -86,6 +86,17 @@ def __repr__(self): | |||
return f"[{self.title} command, status code: {status}, {'in progress.' if not self.is_done else 'finished.'} PID: {self._process.pid}]" | |||
|
|||
|
|||
def run_subprocess(command, folder, check=True) -> subprocess.CompletedProcess: | |||
return subprocess.run( | |||
command.split(), |
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.
This is wrong I think, you really want to pass list of arguments and not split arbitrarily.
For instance on filename
containing spaces it would break:
filename = "my filename.txt
, command is now: `["xxx", "my", "filename.txt"], instead of expected ["xxx", "my filename.txt"].
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.
yes!
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.
Thanks for the review, will adapt the draft
6537230
to
9da3c09
Compare
9da3c09
to
6ae35df
Compare
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.
This is so much nicer, thanks a lot for refactoring those!
|
||
def run_subprocess(command, folder, check=True) -> subprocess.CompletedProcess: | ||
if isinstance(command, str): | ||
logger.error("`run_subprocess` should be called with a list of strings.") |
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.
Why not a hard error?
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.
This looks quite good. Pinging @LysandreJik as a reminder.
logger = get_logger(__name__) | ||
|
||
|
||
def run_subprocess(command, folder, check=True) -> subprocess.CompletedProcess: |
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.
docstring documentation here would be nice.
Thanks for the reminder! Would indeed like to update this and wrap this up. |
af76d67
to
7fe58a8
Compare
Should be good to go! |
The documentation is not available anymore as the PR was closed or merged. |
logger = get_logger(__name__) | ||
|
||
|
||
def run_subprocess( |
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've had bad experiences in including functions in __init__
files since soon it'll cause some circular import issues. Could we move this to a utils/_subprocess.py
file (I don't really feel strongly about the file name)?
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.
Yes, sounds good. Will move it around!
aafa495
to
4a289a6
Compare
@merveenoyan are you aware of why this test might fail?
Failed before and after rebasing on current |
Implement a
run_subprocess
method to remove most of the verbosity introduced bysubprocess.run