improve(Build): Add build script capabilities - #8926
Conversation
bd8e2e8 to
96b77a1
Compare
Greptile SummaryThis PR expands build-script configurability for nested backend builds and containerized development environments.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "fix: github organization as relative pat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A small generic adaptation: Bind-mount `--github-organization` into the cmake_build container when it is a local directory. This allows for nested builds to use the local repositories instead of fetching them. TRITON_REPO_ORGANIZATION already propagates into every nested component configure (e.g. core -> common), so pointing the org at a local repo mirror resolves all clones offline.
+ ruff formatting
020bb7c to
432c610
Compare
Normalize as absolute path for all usages
|
Hi @jcuquemelle - Thank you for the PR! Confirmed on our side: this adds optional Validated through our internal CI (Pipeline ID: 64574726, branch |
| DEFAULT_MIN_COMPUTE_CAPABILITY = "6.0" | ||
|
|
There was a problem hiding this comment.
This value can't be used currently we support 7.5 and higher and we no longer rely on it in favor of CUDA_ARCH_LIST
https://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#gpu-feature-list
| if FLAGS.build_parallel_was_explicit: | ||
| cargs.append( | ||
| cmake_backend_arg( | ||
| "onnxruntime", "TRITON_BUILD_PARALLEL", None, FLAGS.build_parallel | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I would suggest use --extra-backend-cmake-arg instead.
| runargs += docker_runargs() | ||
| # If --github-organization is a local directory (a repo mirror), mount | ||
| # it too. The org propagates into every nested component configure | ||
| # (TRITON_REPO_ORGANIZATION), so pointing it at a local mirror is the | ||
| # only way to resolve deeply-nested clones (e.g. core -> common) | ||
| # offline; FETCHCONTENT_SOURCE_DIR overrides do not reach those builds. | ||
| # FLAGS.github_organization is already normalized as an absolute path | ||
| if FLAGS.github_organization and os.path.isdir(FLAGS.github_organization): | ||
| runargs += [ | ||
| "-v", | ||
| f"{FLAGS.github_organization}:{FLAGS.github_organization}", | ||
| ] |
There was a problem hiding this comment.
I don't think it make sense to mess up git url prefixes with local mounts.
| if FLAGS.cuda_arch_list is not None: | ||
| runargs += ["-e", f'"CUDA_ARCH_LIST={FLAGS.cuda_arch_list}"'] | ||
|
|
There was a problem hiding this comment.
It can be added to override some system defined variable.
Need more context on it.
| def min_cuda_arch(arch_list_str): | ||
| # Lowest compute capability in a CUDA_ARCH_LIST-style string, as a decimal | ||
| # string ("8.6"). Accepts the space/semicolon-separated forms used across | ||
| # the Triton build: "8.6", "86", "75-real", "100f", "PTX". Tokens that don't | ||
| # parse are skipped. Returns None if no token parses. | ||
| if not arch_list_str: | ||
| return None | ||
| arch_list_str = arch_list_str.replace("PTX", "") | ||
| mins = [] | ||
| for tok in re.split(r"[;\s]+", arch_list_str): | ||
| tok = tok.strip() | ||
| if not tok: | ||
| continue | ||
| tok = tok.rstrip("f").removesuffix("-real").removesuffix("-ptx") | ||
| try: | ||
| val = float(tok) | ||
| except ValueError: | ||
| continue | ||
| # "86" means 8.6; "8.6" stays; "100" means 10.0 | ||
| if val >= 100: | ||
| val = val / 10.0 | ||
| elif val >= 20 and val.is_integer(): | ||
| val = val / 10.0 | ||
| mins.append(val) | ||
| return f"{min(mins):.1f}" if mins else None |
There was a problem hiding this comment.
Seams like a CMake codes for NVCC compiler, what is the motivation to add it here?
| docker_host = os.getenv("DOCKER_HOST") | ||
| if docker_host is None: | ||
| return ["-v", "/var/run/docker.sock:/var/run/docker.sock"] | ||
|
|
||
| runargs = ["--network", "host", "-e", f"DOCKER_HOST={docker_host}"] | ||
| if docker_host.startswith("unix://"): | ||
| socket_path = docker_host[len("unix://") :] | ||
| runargs += ["-v", f"{socket_path}:{socket_path}"] | ||
| return runargs |
There was a problem hiding this comment.
@jcuquemelle could you please share your build.py execution call so we can better understand the needs.
3 participants
What does the PR do?
This PR adds several parameters to the main build script (and/or forwards them to ORT bakend build) to add configurability:
--github-organization. This enables having a local clone of needed triton repos and build against them instead of fetching remote repositories, even in nested builds.Checklist
Agreement
<commit_type>: <Title>pre-commit install, pre-commit run --all)Commit Type:
Check the conventional commit type
box here and add the label to the github PR.
Related PRs:
triton-inference-server/onnxruntime_backend#354
Where should the reviewer start?
Only 1 file changed in the PR
Test plan:
Run the build with related options
Tested on internal CI
Caveats:
Only tested orwarding to onnx runtime, other backends could need adaption for these options to be active
Background
We needed this for adaption to our build system's constraints, and to iterate faster by adding a bit of incrementality on ORT binary build
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
None