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.

Sign up for GitHub

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

Jump to bottom

fix: build-secret flag not being set breaking build.py #7993

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

fix: build-secret flag not being set breaking build.py #7993

wants to merge 7 commits into from

Conversation

BenjaminBraunDev
Copy link

Changes the secrets checks in build.py to accurately evaluate the state of the dictionary so as to not set empty docker build flags that cause errors when building the tritonserver image.

Full details in #7992

@BenjaminBraunDev
Fix build-secret flag not being set breaking build.py by changing sec…
1428802 
…rets checks to accurately evaluate the state of the dictionary.
@BenjaminBraunDev BenjaminBraunDev mentioned this pull request Feb 6, 2025
Open
@rmccorm4 rmccorm4 requested review from nv-kmcgill53 and mc-nv and removed request for nv-kmcgill53 February 7, 2025 18:45
@mc-nv mc-nv requested a review from pvijayakrish February 10, 2025 18:38
@mc-nv
Copy link
Contributor

mc-nv commented Feb 10, 2025

@pvijayakrish I believe this change is touching part of code which you introduced recently.
Would you mind to make a review and advise on this change?

pvijayakrish
pvijayakrish approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@pvijayakrish pvijayakrish left a comment

Choose a reason for hiding this comment

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

lgtm!

@nv-kmcgill53
Copy link
Contributor

Hi @BenjaminBraunDev, thank you for your contribution to the Triton Inference Server project. Can you please fill out and sign the Triton CLA agreement so we can accept your contribution. Thanks!

@BenjaminBraunDev
Copy link
Author

Hi @BenjaminBraunDev, thank you for your contribution to the Triton Inference Server project. Can you please fill out and sign the Triton CLA agreement so we can accept your contribution. Thanks!

Sounds good, sent.

@nv-kmcgill53
Copy link
Contributor

Hi @BenjaminBraunDev, we have accepted your CLA and we are able to merge your PR at our discretion. Thanks for your contribution!

@rmccorm4
Copy link
Contributor

@pvijayakrish @nv-kmcgill53 can you merge if you're happy with it?

@rmccorm4 rmccorm4 added the build Issues pertaining to builds label Feb 21, 2025
@BenjaminBraunDev
Copy link
Author

@pvijayakrish @nv-kmcgill53 could you merge this if there is no further feedback?

dmitry-tokarev-nv
dmitry-tokarev-nv approved these changes Feb 26, 2025
View reviewed changes
Copy link

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Choose a reason for hiding this comment

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

LGTM! using Python's true values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvijayakrish pvijayakrish pvijayakrish approved these changes

@dmitry-tokarev-nv dmitry-tokarev-nv dmitry-tokarev-nv approved these changes

@mc-nv mc-nv Awaiting requested review from mc-nv

@nv-kmcgill53 nv-kmcgill53 Awaiting requested review from nv-kmcgill53

Assignees
No one assigned
Labels
build Issues pertaining to builds
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BenjaminBraunDev @mc-nv @nv-kmcgill53 @rmccorm4 @pvijayakrish @dmitry-tokarev-nv