TensorRT packaging relies on nested pip execution

Packaging
1

I’ll admit I felt a little sick in my stomach when I stumbled upon this gem:


class InstallCommand(install):
    def run(self):
        # pip-inside-pip hack ref #3080
        run_pip_command(
            [
                "install",
                "--extra-index-url",
                nvidia_pip_index_url,
                *tensorrt_submodules,
            ],
            subprocess.check_call,
        )

        super().run()

You can see some of the underlying motivation for the pip-ception here:

It seems like the motivation is to dynamically extend index urls to the nvidia indexes.

This also somehow feels like it could lead to bad practices around dependency confusion and extra-index-url

It also wont work for other package managers I presume.

It’s almost better if installation simply failed and printed out instructions on the sdist for how to ensure the nvidia index-url is set if there truly isn’t a better way than nested pip install?

1 Like
2

It seems like they’re using setup.py to configure the user’s system, so that future invocations of Pip will know about their own package indexes.

There’s what I interpret as basically a different version of this, which appears to work by locating Pip’s config file and rewriting it. (And in order to search for the config file, it’s trying to use functionality from pip._internal.configuration…).

IMO this is clearly not how the system is supposed to work. Regardless of whether Pip intends to offer an API for other programs to locate or edit Pip config files, none of this makes sense to wrap into a setup.py or fit into the Setuptools framework. It isn’t actually installing usable Python code (yet). It should be provided as a separate Python script that can just be downloaded and run. (It’s not as if going through setup.py is any more secure than that!)

In this context, it seems like they’re doing it as a step in a larger sdist-building process, which would on its face seem more justifiable. But I agree that this feels quite wrong. There’s only so much that can be done automatically to try to make an sdist work on a remote machine, when you don’t control the initial state of the environment. It also doesn’t seem very polite, to me, that they use this to make Pip grab packages from separate indices that the user didn’t specify on the command line. As a user I should be at least somewhat in control of what domains my machine is connecting to for downloads; I’d expect to follow instructions in a README and explicitly add the package index URLs myself first, instead.

(This previous thread seems related?)

2 Likes
3

It is used as the primary documented approach to install “tensorrt”. It isn’t used as an advanced installation mode unfortunately.

“tensorrt” is an sdist-only metapackage.

4

Yeah that seems pretty horrible. I’m not sure what the point is of raising it here though? Thanks for the heads up, I guess, at least I’ll now know to view any issues raised by users of TensorRT with suspicion…

To get TensorRT to change, someone would have to raise this with the project.

1 Like
5

Nothing expected from this forum. Raised it for awareness so standards-aware people have context of real-world usage.

Perhaps also on the off chance there’s a better recommendation for this package.

I personally think the cleanest approach would be:

  • a tensorrt sdist meta-package that fails and prints out instructions to install “tensorrt-real” wheels from nvidia indexes
  • a tensorrt-real package and wheels on nvidia indexes so that people can install directly from nvidia without the meta-package (a dummy package with the same name that fails would also need to be installed on PyPI)

But I can imagine pushback will be that it will break existing users of “pip install tensorrt”.

3 Likes
6

It’s worth noting that this usage is (basically) unsupported. Python core doesn’t guarantee that you can safely mess with the site-packages of a running interpreter (at the very least you need to clear the import system’s caches), and pip doesn’t guarantee that it can be run concurrently on the same environment.

So IMO it’s fine to ignore this usage as far as standards are concerned. If the TensorRT project would like standard-backed support for their use case, they can propose something, of course.

2 Likes
7

And, I’m also certain that we’d engage with them to figure this out, but I expect like other instances where we have gaps, the maintainers will need to talk to the underlying tooling maintainers about the gaps and describe what they want to do.

1 Like
8

If you look at the packages in the nvidia index then it seems pretty clear what their first problem is which is that they want different wheels for different CUDA versions. That is something that the selector package idea could help with.

1 Like
9

Also related: https://discuss.python.org/t/what-to-do-about-gpus-and-the-built-distributions-that-support-them/

10

Hi from NVIDIA. We’ve been working on a better way that respects existing standards. We aren’t quite ready to share it yet. Watch for a post from Ethan Smith around PyCon time in about two weeks. He’s on vacation right now, and because he developed it, we’d like to give him the chance to share it. We’ll link to it here.

8 Likes