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

PA Migration: Update L0_client_build_variants #7505

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

fpetrini15
Copy link
Contributor

The objective of L0_client_build_variants is to build client libraries/binaries by enabling/disabling various optional features and verify that the build succeeds.

One such set of combinations is to enable/disable optional PA features and verify the build succeeds. This test had been passing (false positive) despite running with a client branch that purged PA from the client repo. Examining the logs closer, it was because CMake was simply ignoring PA-related parameters and repeatedly performing the same standard cc-client/python-client build:

Manually-specified variables were not used by the project:

    TRITON_ENABLE_PERF_ANALYZER
    TRITON_ENABLE_PERF_ANALYZER_C_API
    TRITON_ENABLE_PERF_ANALYZER_TFS
    TRITON_ENABLE_PERF_ANALYZER_TS

These changes modify the script to point to PA source code that recognizes these optional parameters, restoring the test's original purpose.

Ideally, these tests will eventually be moved out of Triton CI entirely and into PA CI.

@fpetrini15 fpetrini15 changed the title Fpetrini update client build variants PA Migration: Update L0_client_build_variants Aug 7, 2024
@rmccorm4 rmccorm4 self-requested a review August 7, 2024 01:33
@rmccorm4
Copy link
Contributor

rmccorm4 commented Aug 7, 2024

I was going to ask if we could detect and raise an error to catch these "unused variables" silent issues, but it seems like a common problem with no clear solution (from 2min of research):

So I suppose there's no quick check we can do on our end, other than parsing the output with some custom logic.

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

LGTM other than adding follow-up ticket comment

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Great description and nice catch 🚀

@fpetrini15 fpetrini15 merged commit a4285ff into main Aug 7, 2024
3 checks passed
@fpetrini15 fpetrini15 deleted the fpetrini-update-client-build-variants branch August 7, 2024 17:01
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.

2 participants