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

Add resolve_model_relative_to_config_file config option #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Legion2
Copy link

@Legion2 Legion2 commented Jan 22, 2024

Currently relative paths to local models are resolved relative to the triton server process. However when deploying models to a central model registry one may not know in advance where the model files will end up in the local file system of an triton inference server container. Therefore relative paths should be used to point from the model.json to the directory containing the actual model files. The relative paths are maintained when deploying models and downloading them from a registry. To make this work relative paths in model.json must be resolved relative to the model.json file location.

This PR adds a new property (resolve_model_relative_to_config_file) to the model.json which allows to enable this relative resolution behavior. By default the old behavior is maintained.

I added test for the new feature, but do not understand how to execute them and where they are executed in ci.

@dyastremsky
Copy link
Contributor

Thank you for submitting this PR! Can you sign a Contributor License Agreement (CLA) so that we can review it?

@oandreeva-nv, do you have any thoughts on whether this belongs in the vLLM backend or in Triton server to be backend-agnostic? I'm not exactly sure what the "model" parameter is used for here.

@Legion2
Copy link
Author

Legion2 commented Apr 10, 2024

@dyastremsky I will check how I can sign the CLA, you are referring to this?

@nnshah1
Copy link
Contributor

nnshah1 commented Apr 26, 2024

@Legion2 , @oandreeva-nv - Alternately we could change the behavior such that path to the model is always relative to model dir and not server process.

I think I would rather not add the parameter - because it seems in general more valuable to change the behavior to be relative to model.json - I don't know of the rationale for keeping it relative to the server process.

@Legion2
Copy link
Author

Legion2 commented Apr 26, 2024

Then we need a heuristic to decide whenever it is a relative path which needs to be resolved. Because the hugging face registry model path could also be interpreted as a relative path.

@oandreeva-nv
Copy link
Collaborator

@Legion2 , I believe vLLM currently can load weights from a local directory, specified as model in `json. Can we close this PR?

@Legion2
Copy link
Author

Legion2 commented Jul 11, 2024

Relative path work, but when I last checked the relative path was resolved to the working dir of the server process. Which makes the config files not portable, because you need to know where the config is placed relative to the server process. But if you want to store the models with config in S3 and someone else downloads and runs the model, the relative path breaks. This issue is about making the paths relative to the config file, so it does not depend on where you place the config and the model.

@oandreeva-nv
Copy link
Collaborator

Got it, do you think there might be any potential security concerns?

@nnshah1
Copy link
Contributor

nnshah1 commented Jul 11, 2024

Then we need a heuristic to decide whenever it is a relative path which needs to be resolved. Because the hugging face registry model path could also be interpreted as a relative path.

I think for this we'd want to clearly distinguish local path from hf path - via a scheme such as file:/// for local or vice versa

@nnshah1
Copy link
Contributor

nnshah1 commented Jul 11, 2024

Got it, do you think there might be any potential security concerns?

I think we would still want to ensure the relative path doesn't escape the model repository root if we can - to avoid any ability for relative path traversal - we can make it relative to config - but again check that the fully resolved path is still within a certain prefix

src/model.py Outdated Show resolved Hide resolved
@Legion2
Copy link
Author

Legion2 commented Jul 11, 2024

This only adds the relative path feature, but absolute paths already work out of the box and I don't think there are any checks to validate where the absolute path is pointing to.

@ozancaglayan
Copy link

Hi there. What's the progress on this? In my use case, I find it very inconvenient to maintain a file path in model.json files which are part of the S3 structure. I dont know how message passing works between triton server and backends both it would be very good if this could be passed through CLI args or environment variables.

@Legion2 Legion2 requested a review from nnshah1 December 16, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants