torch: Stop using _nt_quote_args from distutils #48618

seemethere · 2020-11-30T22:01:43Z

They removed the specific function in Python 3.9 so we should just
remake the function here and use our own instead of relying on hidden
functions from the stdlib

Signed-off-by: Eli Uriegas eliuriegas@fb.com

Fixes #48617

facebook-github-bot

@seemethere has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

seemethere · 2020-11-30T22:07:13Z

torch/utils/cpp_extension.py

@@ -400,7 +418,7 @@ def unix_cuda_flags(cflags):
            # overriding the option if the user explicitly passed it.
            _ccbin = os.getenv("CC")
            if (
-                _ccbin is not None 
+                _ccbin is not None


I guess there was an extra whitespace here. 🤷

Do we have a Flake8 lint for trailing whitespace?

I don't believe we do, but we probably should

facebook-github-bot

@seemethere has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

dr-ci · 2020-11-30T22:15:10Z

💊 CI failures summary and remediations

As of commit b944c06 (more details on the Dr. CI page):

1/3 failures introduced in this PR
2/3 broken upstream at merge base 4fe583e on Dec 01 from 1:08pm to 6:50pm PDT (18 commits; c5f1117 - 25e367e)

1 job timed out:

pytorch_ios_12_0_0_arm64_metal_build

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 22 times.

peterjc123 · 2020-12-01T13:17:04Z

@seemethere distutils.spawn switched to a different implementation in Python 3.9 on Windows. So, escaping may not be needed anymore. But there's another thing that we should really care about. That is, it will behave weirdly if there is an empty string inside the args. See #47460 (comment) for more context.

malfet · 2020-12-01T15:44:28Z

torch/utils/cpp_extension.py

+    for i, arg in enumerate(args):
+        if ' ' in arg:
+            args[i] = '"%s"' % arg
+    return args


Since it's our own function that is expected to work only in python-3.6.2 or newer, why not use list comprehensions and f-strings?

Suggested change

for i, arg in enumerate(args):

if ' ' in arg:

args[i] = '"%s"' % arg

return args

return [f'"{arg}"' if ' ' in arg else arg for arg in args]

This is a direct lift and shift of the original function, wanted to keep it as close to the original as possible to avoid any backwards compat issues

seemethere · 2020-12-01T17:58:29Z

@seemethere distutils.spawn switched to a different implementation in Python 3.9 on Windows. So, escaping may not be needed anymore. But there's another thing that we should really care about. That is, it will behave weirdly if there is an empty string inside the args. See #47460 (comment) for more context.

True, I was trying to get the path of least resistance to make this work and lifting and shifting the function out of stdlib seemed like the most backwards compatible solution

facebook-github-bot

@seemethere has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

They removed the specific function in Python 3.9 so we should just remake the function here and use our own instead of relying on hidden functions from the stdlib Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

facebook-github-bot

@seemethere has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-12-03T01:15:34Z

@seemethere merged this pull request in 780f2b9.

Summary: They removed the specific function in Python 3.9 so we should just remake the function here and use our own instead of relying on hidden functions from the stdlib Signed-off-by: Eli Uriegas <eliuriegas@fb.com> Fixes pytorch#48617 Pull Request resolved: pytorch#48618 Reviewed By: samestep Differential Revision: D25230281 Pulled By: seemethere fbshipit-source-id: 57216af40a4ae4dc8bafcf40d2eb3ba793b9b6e2 (cherry picked from commit 780f2b9) Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

) Summary: They removed the specific function in Python 3.9 so we should just remake the function here and use our own instead of relying on hidden functions from the stdlib Signed-off-by: Eli Uriegas <eliuriegas@fb.com> Fixes #48617 Pull Request resolved: #48618 Reviewed By: samestep Differential Revision: D25230281 Pulled By: seemethere fbshipit-source-id: 57216af40a4ae4dc8bafcf40d2eb3ba793b9b6e2 (cherry picked from commit 780f2b9) Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

Summary: They removed the specific function in Python 3.9 so we should just remake the function here and use our own instead of relying on hidden functions from the stdlib Signed-off-by: Eli Uriegas <eliuriegas@fb.com> Fixes pytorch#48617 Pull Request resolved: pytorch#48618 Reviewed By: samestep Differential Revision: D25230281 Pulled By: seemethere fbshipit-source-id: 57216af40a4ae4dc8bafcf40d2eb3ba793b9b6e2

seemethere requested review from ezyang, fmassa and soumith as code owners November 30, 2020 22:01

seemethere added the module: cpp-extensions Related to torch.utils.cpp_extension label Nov 30, 2020

facebook-github-bot added the cla signed label Nov 30, 2020

seemethere added module: build Build system issues and removed cla signed labels Nov 30, 2020

seemethere requested review from a team, yf225 and glaringlee November 30, 2020 22:02

facebook-github-bot reviewed Nov 30, 2020

View reviewed changes

samestep approved these changes Nov 30, 2020

View reviewed changes

seemethere force-pushed the fix__nt_quote_args branch from aae7935 to b469eca Compare November 30, 2020 22:06

seemethere commented Nov 30, 2020

View reviewed changes

facebook-github-bot reviewed Nov 30, 2020

View reviewed changes

malfet approved these changes Dec 1, 2020

View reviewed changes

facebook-github-bot reviewed Dec 1, 2020

View reviewed changes

seemethere force-pushed the fix__nt_quote_args branch from 7d134d2 to 5221777 Compare December 1, 2020 20:11

seemethere added 2 commits December 1, 2020 14:36

torch: Stop using _nt_quote_args from distutils

4afb822

They removed the specific function in Python 3.9 so we should just remake the function here and use our own instead of relying on hidden functions from the stdlib Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

fix mypy issues

b944c06

Signed-off-by: Eli Uriegas <eliuriegas@fb.com>

seemethere force-pushed the fix__nt_quote_args branch from 5221777 to b944c06 Compare December 1, 2020 22:37

facebook-github-bot reviewed Dec 1, 2020

View reviewed changes

walterddr approved these changes Dec 2, 2020

View reviewed changes

facebook-github-bot closed this in 780f2b9 Dec 3, 2020

facebook-github-bot added the Merged label Dec 3, 2020

This was referenced Dec 3, 2020

[1.7.1] torch: Stop using _nt_quote_args from distutils (#48618) #48768

Merged

[v1.7.1] Release Tracker #47622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

torch: Stop using _nt_quote_args from distutils #48618

torch: Stop using _nt_quote_args from distutils #48618

seemethere commented Nov 30, 2020

facebook-github-bot left a comment

seemethere Nov 30, 2020

samestep Nov 30, 2020

seemethere Nov 30, 2020

facebook-github-bot left a comment

dr-ci bot commented Nov 30, 2020 •

edited

Loading

peterjc123 commented Dec 1, 2020 •

edited

Loading

malfet Dec 1, 2020

seemethere Dec 1, 2020

seemethere commented Dec 1, 2020

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot commented Dec 3, 2020

torch: Stop using _nt_quote_args from distutils #48618

torch: Stop using _nt_quote_args from distutils #48618

Conversation

seemethere commented Nov 30, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

seemethere Nov 30, 2020

Choose a reason for hiding this comment

samestep Nov 30, 2020

Choose a reason for hiding this comment

seemethere Nov 30, 2020

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

dr-ci bot commented Nov 30, 2020 • edited Loading

💊 CI failures summary and remediations

peterjc123 commented Dec 1, 2020 • edited Loading

malfet Dec 1, 2020

Choose a reason for hiding this comment

seemethere Dec 1, 2020

Choose a reason for hiding this comment

seemethere commented Dec 1, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 3, 2020

dr-ci bot commented Nov 30, 2020 •

edited

Loading

peterjc123 commented Dec 1, 2020 •

edited

Loading