Skip to content

fix: Fixed exception when the python executable path has a space in it#267

Merged
ivanmkc merged 1 commit into
googleapis:devfrom
ivanmkc:imkc--training-job-trainingscript-python-packager-fix
Mar 22, 2021
Merged

fix: Fixed exception when the python executable path has a space in it#267
ivanmkc merged 1 commit into
googleapis:devfrom
ivanmkc:imkc--training-job-trainingscript-python-packager-fix

Conversation

@ivanmkc

@ivanmkc ivanmkc commented Mar 20, 2021

Copy link
Copy Markdown
Contributor

In lines (line 768-778) in training_jobs.py:

# Run setup.py to create the source distribution.
setup_cmd = self._SETUP_PY_SOURCE_DISTRIBUTION_CMD.format(
    python_executable=_get_python_executable()
).split()

p = subprocess.Popen(
    args=setup_cmd,
    cwd=trainer_root_path,
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE,
)

If your python executable path has spaces in it, you get an exception:

FileNotFoundError: [Errno 2] No such file or directory: '/Volumes/GoogleDrive/Shared'

which is the part of your executable path before the first space.

This fixes avoids combining the executable and args together, which fixes the problem.

@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 20, 2021
@ivanmkc ivanmkc requested a review from sasha-gitg March 20, 2021 00:06
setup_cmd = self._SETUP_PY_SOURCE_DISTRIBUTION_CMD.format(
python_executable=_get_python_executable()
).split()
setup_cmd = [

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if it's more Pythonic to use append, extend or list comprehension instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@sasha-gitg sasha-gitg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One comment. Thanks!

setup_cmd = self._SETUP_PY_SOURCE_DISTRIBUTION_CMD.format(
python_executable=_get_python_executable()
).split()
setup_cmd = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key word {python_executable} should be removed in the constant above(line 836) because it looks like it goes unused.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I was testing on a different repo and didn't copy it over.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it and now addressing some test failures.

@ivanmkc ivanmkc force-pushed the imkc--training-job-trainingscript-python-packager-fix branch from b1dbb11 to 19c424d Compare March 20, 2021 00:25
@ivanmkc ivanmkc force-pushed the imkc--training-job-trainingscript-python-packager-fix branch from 19c424d to 693bdb9 Compare March 20, 2021 00:54
@ivanmkc ivanmkc merged commit 755e7ad into googleapis:dev Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants