Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
test: Add L0_additional_dependency_dirs #7707
test: Add L0_additional_dependency_dirs #7707
Changes from all commits
712a058
f4796ea
a5e1ec6
ecd0347
3ce9dd2
1e0079d
8b58c7d
68dfc7d
b5733d5
26d6489
3f17fab
d876698
c5ab25f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this feature is agnostic to the backend, so having TRT models in this test will probably make it more painful to maintain or run locally than necessary. In your follow-up ticket TPRD-244, I would suggest revisiting this.
For example running CI with the
BACKENDS="python"
or something would have no effect on this test and fail looking for TRT models.Instead I think TRT backend specific tests could add a test case for this feature if there's a specific TRT related flow it might be used for, and this test could be generic to something easier or more portable like a python model.
This would make it easier to compartmentalize running the tests on scenarios where it would be painful to get the TRT models for, or migrate to other runners, other versions, etc without compatibility issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the main use case driving the feature is intended for use with TRT backend for now, it's fine - just food for thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose TRT for 2 reasons:
The python backend does not depend on any implicit dependencies outside of what exists in its corresponding backend directory and those dependencies must remain in that folder since their path relative to
libtriton_python.so
matters. I agree that having any other than TRT would be more maintainable, but I'd have to think on how I'd introduce an additional dependency to the python backend specifically for this test.