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

Fix sdist installation #3080

Merged
merged 15 commits into from Jul 14, 2023
99 changes: 77 additions & 22 deletions python/packaging/frontend_sdist/setup.py
Expand Up @@ -15,42 +15,96 @@
# limitations under the License.
#

import os
import subprocess
import sys

from setuptools import setup
from setuptools.command.install import install
import subprocess as sp

tensorrt_module = "##TENSORRT_MODULE##"
tensorrt_version = "##TENSORRT_PYTHON_VERSION##"
tensorrt_submodules = [
"{}_libs=={}".format(tensorrt_module, tensorrt_version),
"{}_bindings=={}".format(tensorrt_module, tensorrt_version),
]


class InstallCommand(install):
def run(self):
def install_dep(package_name):
status = sp.run(
[
sys.executable,
"-m",
"pip",
"install",
"{:}==##TENSORRT_PYTHON_VERSION##".format(package_name),
"--index-url",
"https://pypi.nvidia.com",
]
)
status.check_returncode()

install_dep("{:}_libs".format(tensorrt_module))
install_dep("{:}_bindings".format(tensorrt_module))

install.run(self)
# pip-inside-pip hack ref #3080
subprocess.check_call(
[
"{}/bin/pip".format(sys.exec_prefix),
"install",
"--extra-index-url",
"https://pypi.nvidia.com",
*tensorrt_submodules,
]
)

super().run()


def pip_config_list():
Copy link
Collaborator

@ttyio ttyio Jul 12, 2023

Choose a reason for hiding this comment

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

Could we replace the function with below code, to solve some issue we see in internal CI/CD, thanks!

def run_pip_command(args, call_func):
    try:
        return call_func([sys.executable, "-m", "pip"] + args)
    except subprocess.CalledProcessError:
        return call_func([os.path.join(sys.exec_prefix, "bin", "pip")] + args)

def pip_config_list():
    """Get the current pip config (env vars, config file, etc)."""
    return run_pip_command(["config", "list"], subprocess.check_output).decode()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"""Get the current pip config (env vars, config file, etc)."""
return subprocess.check_output(
[
"{}/bin/pip".format(sys.exec_prefix),
"config",
"list",
]
).decode()


def parent_command_line():
"""Get the command line of the parent PID."""
pid = os.getppid()
# try retrieval using psutil
try:
import psutil
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the install base looks like for this Python module, but it does look like the best solution if available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it has zero dependencies, but not all platform specific wheels are available and building it from source will fail e.g. on graviton processors ref giampaolo/psutil#2103


return " ".join(psutil.Process(pid).cmdline())
except ModuleNotFoundError:
pass
# fall back to shell
try:
return (
subprocess.check_output(["ps", "-p", str(pid), "-o", "command"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding --no-headers avoids the need for the split(). Checking both CentOS 7 and Ubuntu 18.04 containers this option has been supported since at least then and TRT supports those OSes and newer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.decode()
.split("\n")[1]
)
except subprocess.CalledProcessError:
return ""


# use pip-inside-pip hack only if the nvidia index is not set in the environment
if "pypi.nvidia.com" in pip_config_list() or "pypi.nvidia.com" in parent_command_line():
Copy link
Contributor

Choose a reason for hiding this comment

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

A recent internal (and unrelated) discussion revealed we may want one additional condition here. In order to test different versions or from staging servers (other than pypi.nvidia.com) a full override may be desired. Basically a "manage the source and installation of dependencies yourself" option. I think one way to handle this would be to check an environment variable when set assumes you have set --extra-index-url to something you know works, which could be something other than pypi.nvidia.com. This may be useful externally as well if someone is running their own internal PyPI server and mirroring our wheel/sdist files as-is. @pranavm-nvidia what do you think about this approach and calling this environment variable something like NVIDIA_TENSORRT_DISABLE_INTERNAL_PIP?

Copy link
Contributor Author

@ddelange ddelange Jul 6, 2023

Choose a reason for hiding this comment

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

how about we make NVIDIA_PIP_INDEX_URL configurable? defaulting to https://pypi.nvidia.com, but it could be overwritten with custom index urls or set it to an empty string to always pass the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think NVIDIA_TENSORRT_DISABLE_INTERNAL_PIP is still useful even if we make the NVIDIA pip index URL configurable. There could be a case where there are no extra indexes used (not sure how it would work, but I imagine it would be possible somehow).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

install_requires = tensorrt_submodules
cmdclass = {}
else:
install_requires = []
cmdclass = {"install": InstallCommand}


setup(
name=tensorrt_module,
version="##TENSORRT_PYTHON_VERSION##",
version=tensorrt_version,
description="A high performance deep learning inference library",
long_description="A high performance deep learning inference library",
long_description="""A high performance deep learning inference library

To install, please execute the following:
```
pip install tensorrt --extra-index-url https://pypi.nvidia.com
```
Or put the index URL in the (comma-separated) PIP_EXTRA_INDEX_URL environment variable:
```
export PIP_EXTRA_INDEX_URL=https://pypi.nvidia.com
pip install tensorrt
```
When the extra index url is not found, a nested `pip install` will run with the extra index url hard-coded.
ddelange marked this conversation as resolved.
Show resolved Hide resolved
""",
long_description_content_type="text/markdown",
author="NVIDIA Corporation",
license="Proprietary",
classifiers=[
Expand All @@ -59,12 +113,13 @@ def install_dep(package_name):
"Programming Language :: Python :: 3",
],
packages=[tensorrt_module],
install_requires=install_requires,
cmdclass=cmdclass,
extras_require={"numpy": "numpy"},
package_data={tensorrt_module: ["*.so*", "*.pyd", "*.pdb"]},
include_package_data=True,
zip_safe=True,
keywords="nvidia tensorrt deeplearning inference",
url="https://developer.nvidia.com/tensorrt",
download_url="https://github.com/nvidia/tensorrt/tags",
cmdclass={"install": InstallCommand},
)