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

Bring back Python backend based PyTorch backend #117

Merged
merged 13 commits into from
Jan 11, 2024
Merged

Conversation

kthui
Copy link
Contributor

@kthui kthui commented Nov 14, 2023

@kthui kthui force-pushed the jacky-python-based-pytorch branch from 22e5150 to 990aeb8 Compare November 14, 2023 17:12
@kthui kthui force-pushed the jacky-python-based-pytorch branch from 990aeb8 to 8d50071 Compare November 14, 2023 18:25
@kthui kthui marked this pull request as ready for review November 14, 2023 18:40


def _get_model_path(config):
filenames = ["model.py", "model.pt"]
Copy link
Contributor

@rmccorm4 rmccorm4 Nov 27, 2023

Choose a reason for hiding this comment

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

If I am understanding the whole flow correctly:

  1. The runtime flag for python based implementation in Triton core will look for model.py (configurable by user, but would typically be called model.py) in model/version repo, and then backend repo (in that order)
  2. The python runtime for pytorch backend supports loading pytorch NN module style models as a model.py file

Assume the following setup:

models/
-- resnet50
    -- config.pbtxt   # runtime: "model.py"
    -- 1/
        -- model.py # resnet50 pytorch NN implementation

backends/
-- pytorch/
    -- model.py # python-based backend implementation

Since the core looks in model repo first, if you tried to load a pytorch models/resnet50/1/model.py (nn module file) that uses the Triton python runtime pytorch implementation (/opt/tritonserver/backends/pytorch/model.py) -- Triton core would probably try to load this models/resnet50/1/model.py as the backend implementation first and fail, right?

Copy link
Contributor Author

@kthui kthui Nov 28, 2023

Choose a reason for hiding this comment

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

I guess this might be the most confusing part. If a model wants to load on top of Python backend, the backend_libdir and backend_libpath will initially point to the C++ runtime of the Python backend. After that, the backend_libdir will be "updated" to point to the Python runtime (i.e. /opt/tritonserver/backends/pytorch/model.py), without modifying backend_libpath.

The reason why the "update" logic is always correct is the Python runtime path is always assembled from backend_dir and model.py, where backend_dir always points to /opt/tritonserver/backends/pytorch, so the eventual backend_libdir can only be /opt/tritonserver/backends/pytorch/model.py for example.

However, you are right if the user does not specify the runtime (or filled in by autocomplete), the latter runtime resolution step can find the wrong file as outlined, but this should not be an issue for our backends (vLLM and PyTorch) because the autocomplete will always fill the correct runtime. This will be an issue for custom Python based backends.

Copy link
Contributor Author

@kthui kthui Nov 28, 2023

Choose a reason for hiding this comment

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

I think we should rename the runtime from model.py to backend.py to avoid any ambiguity and allow for better search logic, but model.py is used since vLLM, so it will be a behavioral change if we do so.

Otherwise, we will have to limit the search for Python based runtime to only within backend_dir (i.e. /opt/tritonserver/backends/pytorch)

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
Contributor Author

Choose a reason for hiding this comment

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

Issue is addressed, with commit up to triton-inference-server/core@f502bfc point

Copy link
Contributor

Choose a reason for hiding this comment

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

Try to avoid force pushing if possible, as the commit referenced previously is no longer valid.

Copy link
Contributor

@rmccorm4 rmccorm4 Jan 10, 2024

Choose a reason for hiding this comment

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

Can you clarify the current behavior now?

For python-based backends, it will only look for model.py in the backend directory.

For C++ backends, will it still look in (version_dir, model_dir, backend_dir)?

  • how does this interact with escape logic?
  • can you verify running a custom backend with shared library located in the model folder still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For python-based backends, it will only look for model.py in the backend directory.
For C++ backends, will it still look in (version_dir, model_dir, backend_dir)?

Yes.

how does this interact with escape logic?

The backend_libdir will be one of the (version_dir, model_dir, backend_dir) path. The runtime is essentially the backend_libname, which <backend_libdir>/<runtime> is the backend_libpath. At the end after everything are determined, there is a one time check to ensure the backend_libpath is within the backend_libdir. If the runtime tries to escape from the backend_libdir (i.e. runtime = "../my_backend_lib.so" -> backend_libpath = "backend_dir/../my_backend_lib.so"), this will be caught.

can you verify running a custom backend with shared library located in the model folder still works?

Yes. I think the L0_lifecycle covers such case, and it is part of the CI run. It copies the libtriton_identity.so into the model folder, see cp libtriton_identity.so models/identity_zero_1_int32/1/. line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also ran L0_passive_instance on the test container from CI and it passed, which uses a custom C++ backend: https://github.com/triton-inference-server/server/blob/main/qa/L0_passive_instance/models/distributed_int32_int32_int32/config.pbtxt#L28

conda install -c conda-forge libstdcxx-ng=12 -y

# install PyTorch
conda install pytorch torchvision torchaudio pytorch-cuda=12.1 -c pytorch -c nvidia -y
Copy link
Contributor

Choose a reason for hiding this comment

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

if we don't specify version 12.1 for pytorch-cuda, will it still install the latest stable version?

Copy link
Contributor Author

@kthui kthui Dec 8, 2023

Choose a reason for hiding this comment

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

Without specifying version 12.1 for pytorch-cuda worked locally. I am ok with removing the specified version. My only concern is why PyTorch suggests setting pytorch-cuda=12.1 at the first place:

conda install pytorch torchvision torchaudio pytorch-cuda=12.1 -c pytorch -c nvidia

Would it break in the future? Is it only a mean to lock CUDA to 12.1?
https://pytorch.org/get-started/locally/ (Stable (2.1.1) -> Linux -> Conda -> Python -> CUDA 12.1 -> get the command)

Copy link
Contributor Author

Choose a reason for hiding this comment

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



def _get_model_path(config):
filenames = ["model.py", "model.pt"]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to support <XXX>.pt2 ? i.e result of torch.export : https://pytorch.org/docs/stable/export.html#serialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we add .pt2 support as a follow-up ticket? It is because I think it depends on torch.export which is not part of the previous "platform handler", so we do not introduce additional risks for this "bring back platform handler" ticket and get some functionality in sooner than later. (unless .pt2 requires behavioral changes if introduced separately?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me. Can we add this note as a FIXME? relevant ticket is: https://jirasw.nvidia.com/browse/DLIS-5694

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CMakeLists.txt Outdated
@@ -504,6 +505,21 @@ install(
${INSTALL_CONFIGDIR}
)

if (${TRITON_PYTORCH_ENABLE_PYTHON_RUNTIME})
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that we'll include the runtime by default? I believe the environment size could be significant since it contains CUDA, PyTorch, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the behavior can be easily changed. The current size of the conda pack is 3.00 GB.

src/model.py Outdated
Comment on lines 38 to 41
# triton_python_backend_utils is available in every Triton Python model. You
# need to use this module to create inference requests and responses. It also
# contains some utility functions for extracting information from model_config
# and converting Triton input/output types to numpy types.
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kthui kthui force-pushed the jacky-python-based-pytorch branch from 3d6d797 to 9aa6b41 Compare January 8, 2024 23:40
model_repository/
`-- model_directory
|-- 1
| |-- model.py
Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably a little bit misleading since we don't always require both model.py and model.pt (e.g., if torchscript is provided only model.pt is required as you mentioned).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spotting! I created different sections for PyTorch 2.0 and TorchScript model layout. Clarify model layout between PyTorch and TorchScript

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

This PR generally looks good to me - just looking for some clarifications to close this thread: https://github.com/triton-inference-server/pytorch_backend/pull/117/files#r1447954567

@kthui kthui requested a review from rmccorm4 January 11, 2024 00:28
@kthui kthui merged commit 7468381 into main Jan 11, 2024
1 check passed
@kthui kthui deleted the jacky-python-based-pytorch branch January 11, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants