Skip to content

build: update build.py to pass versions as input parameter and convert version map to dictionary #7500

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

Merged
merged 52 commits into from
Nov 21, 2024

Conversation

nvda-mesharma
Copy link
Contributor

@nvda-mesharma nvda-mesharma commented Aug 5, 2024

What does the PR do?

  1. Replace TRITON_VERSION_MAP with a dictionary of key and values instead of remembering indexes throughout the script.
  2. Add ability to pass backend version (start with vllm version) instead of hard-coding in the build.py
    example - https://github.com/triton-inference-server/vllm_backend/blob/mesharma-ci/ci/build/build_source.sh#L60

Checklist

  • [ x ] PR title reflects the change and is of format <commit_type>: <Title>
  • [ x ] Changes are described in the pull request.
  • [ x ] Related issues are referenced.
  • [ x ] Populated github labels field
  • [ x ] Added test plan and verified test passes.
  • [ x ] Verified that the PR passes existing CI.
  • [ x] Verified copyright is correct on all changed files.
  • [ x] Added succinct git squash message before merging ref.
  • [ x] All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • [ x ] build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

[Not ready for review, not needed to merge this change] triton-inference-server/vllm_backend#49

Where should the reviewer start?

all changes are contained within build.py script

Test plan:

  1. Verified that the changes work with vllm_backedn ci build.
    Build: Trigger CI for new vllm_backend Triton releases vllm_backend#49
    All actions are successful and pipeline checks have also passed.
  2. This is a no-op on release build process

CI Pipeline ID:

17157949

Caveats:

N/A

Background

I was unable to set any specific vllm version while triggering a build and test CI pipeline from vllm_backend repository due to the hard-coding in this build.py script.

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

N/A

@oandreeva-nv
Copy link
Contributor

Is it possible to start a new pipeline for this PR to check if it is goes well? + should we test newly added flags?

@nvda-mesharma
Copy link
Contributor Author

nvda-mesharma commented Oct 1, 2024

I've tested the changes via this pipeline 18898001
All checks have passed here: triton-inference-server/vllm_backend#49
Pipeline link can also be found here

@nvda-mesharma nvda-mesharma changed the title build: update build.py to pass vllm versions as input parameter and convert version map to dictionary build: update build.py to pass versions as input parameter and convert version map to dictionary Nov 20, 2024
Co-authored-by: Olga Andreeva <124622579+oandreeva-nv@users.noreply.github.com>
@oandreeva-nv
Copy link
Contributor

oandreeva-nv commented Nov 20, 2024

I'm fine with these changes, just clarify help messages, as I mentioned here

If @mc-nv and @nv-kmcgill53 have no objections and there are no disturbances with our current CI, i.e. not the vllm specific, we should be fine.

Copy link
Contributor

@nv-kmcgill53 nv-kmcgill53 left a comment

Choose a reason for hiding this comment

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

We should be using the FLAGS for all of the versioning instead of the DEFAULT_TRITON_VERSION_MAP since each of the values in the map can be overwritten as passed arguments to build.py. You are defaulting the FLAG values to use the map as well. I don't really want bidirectional assignment from the FLAGS and the map, too confusing.

nvda-mesharma and others added 2 commits November 20, 2024 17:10
Co-authored-by: Kyle McGill <101670481+nv-kmcgill53@users.noreply.github.com>
@nvda-mesharma
Copy link
Contributor Author

Since there were some recent changes, I went ahead and ran a new pipeline to ensure that things were still working as expected
.../pipelines/20628717

@nvda-mesharma nvda-mesharma merged commit 9175390 into main Nov 21, 2024
3 checks passed
@nvda-mesharma nvda-mesharma deleted the mesharma-ci branch November 21, 2024 18:48
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