-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: Minor improvements to build.py #8362
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
Open
kpedro88
wants to merge
5
commits into
triton-inference-server:main
Choose a base branch
from
kpedro88:buildpy_fixes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
378d59a
arg to disable docker cache
kpedro88 e57b755
add arg to override default repo tag for all dependencies
kpedro88 ce53f43
add explicit buildbase option to use temporary container for backend …
kpedro88 82eac33
allow changing github org for individual backends
kpedro88 f940474
update build docs
kpedro88 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -32,6 +32,7 @@ | |||||
import os.path | ||||||
import pathlib | ||||||
import platform | ||||||
import re | ||||||
import stat | ||||||
import subprocess | ||||||
import sys | ||||||
|
@@ -688,17 +689,17 @@ def onnxruntime_cmake_args(images, library_paths): | |||||
) | ||||||
|
||||||
if target_platform() == "windows": | ||||||
if "base" in images: | ||||||
if "buildbase" in images: | ||||||
cargs.append( | ||||||
cmake_backend_arg( | ||||||
"onnxruntime", "TRITON_BUILD_CONTAINER", None, images["base"] | ||||||
"onnxruntime", "TRITON_BUILD_CONTAINER", None, images["buildbase"] | ||||||
) | ||||||
) | ||||||
else: | ||||||
if "base" in images: | ||||||
if "buildbase" in images: | ||||||
cargs.append( | ||||||
cmake_backend_arg( | ||||||
"onnxruntime", "TRITON_BUILD_CONTAINER", None, images["base"] | ||||||
"onnxruntime", "TRITON_BUILD_CONTAINER", None, images["buildbase"] | ||||||
) | ||||||
) | ||||||
else: | ||||||
|
@@ -754,17 +755,17 @@ def openvino_cmake_args(): | |||||
) | ||||||
] | ||||||
if target_platform() == "windows": | ||||||
if "base" in images: | ||||||
if "buildbase" in images: | ||||||
cargs.append( | ||||||
cmake_backend_arg( | ||||||
"openvino", "TRITON_BUILD_CONTAINER", None, images["base"] | ||||||
"openvino", "TRITON_BUILD_CONTAINER", None, images["buildbase"] | ||||||
) | ||||||
) | ||||||
else: | ||||||
if "base" in images: | ||||||
if "buildbase" in images: | ||||||
cargs.append( | ||||||
cmake_backend_arg( | ||||||
"openvino", "TRITON_BUILD_CONTAINER", None, images["base"] | ||||||
"openvino", "TRITON_BUILD_CONTAINER", None, images["buildbase"] | ||||||
) | ||||||
) | ||||||
else: | ||||||
|
@@ -801,9 +802,9 @@ def dali_cmake_args(): | |||||
|
||||||
def fil_cmake_args(images): | ||||||
cargs = [cmake_backend_enable("fil", "TRITON_FIL_DOCKER_BUILD", True)] | ||||||
if "base" in images: | ||||||
if "buildbase" in images: | ||||||
cargs.append( | ||||||
cmake_backend_arg("fil", "TRITON_BUILD_CONTAINER", None, images["base"]) | ||||||
cmake_backend_arg("fil", "TRITON_BUILD_CONTAINER", None, images["buildbase"]) | ||||||
) | ||||||
else: | ||||||
cargs.append( | ||||||
|
@@ -1787,6 +1788,11 @@ def create_docker_build_script(script_name, container_install_dir, container_ci_ | |||||
"--pull", | ||||||
] | ||||||
|
||||||
if FLAGS.no_container_cache: | ||||||
baseargs += [ | ||||||
"--no-cache", | ||||||
] | ||||||
|
||||||
# Windows docker runs in a VM and memory needs to be specified | ||||||
# explicitly (at least for some configurations of docker). | ||||||
if target_platform() == "windows": | ||||||
|
@@ -2057,14 +2063,14 @@ def tensorrtllm_postbuild(cmake_script, repo_install_dir, tensorrtllm_be_dir): | |||||
def backend_build( | ||||||
be, | ||||||
cmake_script, | ||||||
tag, | ||||||
tag_org, | ||||||
build_dir, | ||||||
install_dir, | ||||||
github_organization, | ||||||
images, | ||||||
components, | ||||||
library_paths, | ||||||
): | ||||||
tag, github_organization = tag_org | ||||||
repo_build_dir = os.path.join(build_dir, be, "build") | ||||||
repo_install_dir = os.path.join(build_dir, be, "install") | ||||||
|
||||||
|
@@ -2077,8 +2083,8 @@ def backend_build( | |||||
if be == "tensorrtllm": | ||||||
github_organization = ( | ||||||
"https://github.com/NVIDIA" | ||||||
if "triton-inference-server" in FLAGS.github_organization | ||||||
else FLAGS.github_organization | ||||||
if "triton-inference-server" in github_organization | ||||||
else github_organization | ||||||
) | ||||||
repository_name = "TensorRT-LLM" | ||||||
cmake_script.gitclone(repository_name, tag, be, github_organization) | ||||||
|
@@ -2127,11 +2133,11 @@ def backend_build( | |||||
def backend_clone( | ||||||
be, | ||||||
clone_script, | ||||||
tag, | ||||||
tag_org, | ||||||
build_dir, | ||||||
install_dir, | ||||||
github_organization, | ||||||
): | ||||||
tag, github_organization = tag_org | ||||||
clone_script.commentln(8) | ||||||
clone_script.comment(f"'{be}' backend") | ||||||
clone_script.comment("Delete this section to remove backend from build") | ||||||
|
@@ -2467,6 +2473,12 @@ def enable_all(): | |||||
required=False, | ||||||
help="Do not use Docker --pull argument when building container.", | ||||||
) | ||||||
parser.add_argument( | ||||||
"--no-container-cache", | ||||||
action="store_true", | ||||||
required=False, | ||||||
help="Use Docker --no-cache argument when building container.", | ||||||
) | ||||||
parser.add_argument( | ||||||
"--container-memory", | ||||||
default=None, | ||||||
|
@@ -2580,6 +2592,12 @@ def enable_all(): | |||||
required=False, | ||||||
help='Use specified Docker image in build as <image-name>,<full-image-name>. <image-name> can be "base", "gpu-base", or "pytorch".', | ||||||
) | ||||||
parser.add_argument( | ||||||
"--use-buildbase", | ||||||
default=False, | ||||||
action="store_true", | ||||||
help='Use local temporary "buildbase" Docker image as "base" image to build backends', | ||||||
) | ||||||
|
||||||
parser.add_argument( | ||||||
"--enable-all", | ||||||
|
@@ -2659,7 +2677,7 @@ def enable_all(): | |||||
"--backend", | ||||||
action="append", | ||||||
required=False, | ||||||
help='Include specified backend in build as <backend-name>[:<repo-tag>]. If <repo-tag> starts with "pull/" then it refers to a pull-request reference, otherwise <repo-tag> indicates the git tag/branch to use for the build. If the version is non-development then the default <repo-tag> is the release branch matching the container version (e.g. version YY.MM -> branch rYY.MM); otherwise the default <repo-tag> is "main" (e.g. version YY.MMdev -> branch main).', | ||||||
help='Include specified backend in build as <backend-name>[:<repo-tag>][:<org>]. If <repo-tag> starts with "pull/" then it refers to a pull-request reference, otherwise <repo-tag> indicates the git tag/branch to use for the build. If the version is non-development then the default <repo-tag> is the release branch matching the container version (e.g. version YY.MM -> branch rYY.MM); otherwise the default <repo-tag> is "main" (e.g. version YY.MMdev -> branch main). <org> allows using a forked repository instead of the default --github-organization value.', | ||||||
) | ||||||
parser.add_argument( | ||||||
"--repo-tag", | ||||||
|
@@ -2727,6 +2745,12 @@ def enable_all(): | |||||
default=DEFAULT_TRITON_VERSION_MAP["upstream_container_version"], | ||||||
help="This flag sets the upstream container version for Triton Inference Server to be built. Default: the latest released version.", | ||||||
) | ||||||
parser.add_argument( | ||||||
"--default-repo-tag", | ||||||
required=False, | ||||||
default=None, | ||||||
help="Override the calculated default-repo-tag value", | ||||||
) | ||||||
parser.add_argument( | ||||||
"--ort-version", | ||||||
required=False, | ||||||
|
@@ -2855,6 +2879,8 @@ def enable_all(): | |||||
cver = FLAGS.triton_container_version | ||||||
if not cver.endswith("dev"): | ||||||
default_repo_tag = "r" + cver | ||||||
if FLAGS.default_repo_tag: | ||||||
default_repo_tag = FLAGS.default_repo_tag | ||||||
log("default repo-tag: {}".format(default_repo_tag)) | ||||||
|
||||||
# For other versions use the TRITON_VERSION_MAP unless explicitly | ||||||
|
@@ -2874,11 +2900,14 @@ def enable_all(): | |||||
# Initialize map of backends to build and repo-tag for each. | ||||||
backends = {} | ||||||
for be in FLAGS.backend: | ||||||
parts = be.split(":") | ||||||
pattern = r"(https?:\/\/[^\s:]+)|:" | ||||||
parts = list(filter(None,re.split(pattern, be))) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing space after comma in function call. Should be
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
if len(parts) == 1: | ||||||
parts.append(default_repo_tag) | ||||||
log('backend "{}" at tag/branch "{}"'.format(parts[0], parts[1])) | ||||||
backends[parts[0]] = parts[1] | ||||||
if len(parts) == 2: | ||||||
parts.append(FLAGS.github_organization) | ||||||
log('backend "{}" at tag/branch "{}" from org "{}"'.format(parts[0], parts[1], parts[2])) | ||||||
backends[parts[0]] = parts[1:] | ||||||
|
||||||
if "vllm" in backends: | ||||||
if "python" not in backends: | ||||||
|
@@ -2926,6 +2955,11 @@ def enable_all(): | |||||
) | ||||||
log('image "{}": "{}"'.format(parts[0], parts[1])) | ||||||
images[parts[0]] = parts[1] | ||||||
if FLAGS.use_buildbase: | ||||||
images["buildbase"] = "tritonserver_buildbase" | ||||||
else: | ||||||
if "base" in images: | ||||||
images["buildbase"] = images["base"] | ||||||
|
||||||
# Initialize map of library paths for each backend. | ||||||
library_paths = {} | ||||||
|
@@ -3084,7 +3118,6 @@ def enable_all(): | |||||
backends[be], | ||||||
script_build_dir, | ||||||
script_install_dir, | ||||||
github_organization, | ||||||
) | ||||||
else: | ||||||
backend_build( | ||||||
|
@@ -3093,7 +3126,6 @@ def enable_all(): | |||||
backends[be], | ||||||
script_build_dir, | ||||||
script_install_dir, | ||||||
github_organization, | ||||||
images, | ||||||
components, | ||||||
library_paths, | ||||||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern and splitting logic is complex and may be difficult to maintain. Consider extracting this parsing logic into a separate function with clear documentation about the expected input format and returned structure.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. My followup branch is not quite finished yet, but involves a more thorough redesign that will avoid the need for any custom parsing.