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

test: Add L0_additional_dependency_dirs #7707

Merged
merged 13 commits into from
Oct 23, 2024
Merged

Conversation

fpetrini15
Copy link
Contributor

@fpetrini15 fpetrini15 commented Oct 15, 2024

What does the PR do?

Add testing for new Windows feature that supports additional dependency directories during backend loading.

Checklist

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

Commit Type:

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

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

Core: triton-inference-server/core#400

Where should the reviewer start?

Check Core PR first

Test plan:

L0_additional_dependency_dirs--base

  • CI Pipeline ID:

[WIP]

@fpetrini15 fpetrini15 added the PR: test Adding missing tests or correcting existing test label Oct 15, 2024
@fpetrini15 fpetrini15 force-pushed the fpetrini-additional-dep-dir branch from e01462f to 68dfc7d Compare October 18, 2024 19:30
@fpetrini15 fpetrini15 requested a review from GuanLuo October 21, 2024 16:40

This is an optional Windows feature that enables Triton to search custom
dependency directories when loading a specific backend. The user can input
these directories as a string of semicolon-separated paths. These directories
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention that the input must have trailing ";"

qa/L0_additional_dependency_dirs/test.sh Outdated Show resolved Hide resolved
@fpetrini15 fpetrini15 requested a review from GuanLuo October 22, 2024 20:20
STALE_DEPENDENCY_PATH="C:/ci_test_deps/24.05"
CUSTOM_DEPENDENCY_DIR=${MODELDIR}/custom_dependency_location
STALE_DEPENDENCY_DIR=${MODELDIR}/stale_dependency_location
TRT_MODEL_DIR="C:/tmp/24.07_trt_models"
Copy link
Contributor

@rmccorm4 rmccorm4 Oct 23, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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:

  1. The TRT backend was the motivating backend for this feature (as you say)
  2. The backend requires implicit dependencies that are expected to exist outside of the backend directory.

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.

@fpetrini15 fpetrini15 merged commit c91e412 into main Oct 23, 2024
3 checks passed
@fpetrini15 fpetrini15 deleted the fpetrini-additional-dep-dir branch October 23, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: test Adding missing tests or correcting existing test
Development

Successfully merging this pull request may close these issues.

3 participants