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

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

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

nvda-mesharma
Copy link

@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

@nvda-mesharma nvda-mesharma marked this pull request as draft August 5, 2024 19:12
@nvda-mesharma nvda-mesharma changed the title Update build.py to pass vllm versions as input parameter and convert version map to dictionary Build: update build.py to pass vllm versions as input parameter and convert version map to dictionary Aug 5, 2024
@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 vllm versions as input parameter and convert version map to dictionary Aug 5, 2024
@nvda-mesharma nvda-mesharma marked this pull request as ready for review August 5, 2024 19:30
build.py Show resolved Hide resolved
@nv-kmcgill53
Copy link
Contributor

nv-kmcgill53 commented Aug 5, 2024

Not asking for change on this just bringing up a limitation of python. There is no idea of const in the language and that was the point of using a tuple instead of a map. tuples being immutable expressed that this collection of versions should be used together. But someone could just overwrite the original tuple anyway to get around this anyway. I guess the way to work around this would have been to define named indices into the tuple but both implementations are equal imo.

Again, not rejecting the change, just pointing out the reason for the previous implementation in case this is important for some reason I have not thought of.

build.py Show resolved Hide resolved
@nvda-mesharma nvda-mesharma marked this pull request as draft August 6, 2024 19:47
@nvda-mesharma nvda-mesharma marked this pull request as ready for review August 6, 2024 20:15
@nvda-mesharma nvda-mesharma marked this pull request as draft August 6, 2024 20:20
@nvda-mesharma nvda-mesharma marked this pull request as ready for review August 6, 2024 21:55
"--triton-container-version",
required=False,
default=DEFAULT_TRITON_VERSION_MAP["triton_container_version"],
help="Provide any released version of project.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the help message make sense, I don't think we release any dev versions

Copy link
Author

Choose a reason for hiding this comment

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

Can you suggest a better help message

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this parameter indicates current development version of the container, not necessary a released version. Based on this, in my vision, the parameter should be something like triton-development-version, and help message would be Provide the current development version of the container. I'm not sure how relevant it would be to provide the past development versions and how much confusion this would introduce to users. Do we really need to expose this parameter? @nv-kmcgill53 and @mc-nv , what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Any kind of logic could be added as an expansion for build.py.
My center of thoughts isn't changed, build.py requires simplification, any expansion need to be reasonable.

I agree that more moving parts we are adding the more complexity it will bring over time.

Copy link
Author

Choose a reason for hiding this comment

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

afaik, this parameter can be used for dev and prod versions of triton container. A better help text for it would be "Provide any development or released version of project"

build.py Show resolved Hide resolved
@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
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

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