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

feat: Incorporate "additional-dependency-dirs" #400

Merged
merged 6 commits into from
Oct 23, 2024

Conversation

fpetrini15
Copy link
Contributor

@fpetrini15 fpetrini15 commented Oct 15, 2024

What does the PR do?

Adds core support for a new "additional-dependency-dirs" configuration option that will add user-specified directories to the search path 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:

Server: triton-inference-server/server#7707

  • CI Pipeline ID:

[WIP]

@fpetrini15 fpetrini15 added the PR: feat A new feature label Oct 15, 2024
@fpetrini15 fpetrini15 changed the title test: Incorporate "additional-dependency-dirs" feat: Incorporate "additional-dependency-dirs" Oct 15, 2024
src/shared_library.h Outdated Show resolved Hide resolved
src/shared_library.cc Outdated Show resolved Hide resolved
src/shared_library.cc Show resolved Hide resolved
@mc-nv
Copy link
Contributor

mc-nv commented Oct 16, 2024

I'm against managing environment variables out of the source code.
It must go through the code scanning tool before merge, NSPECT, VERACODE etc.

@fpetrini15 fpetrini15 force-pushed the fpetrini-additional-dep-dir branch from ac2623a to 74d1887 Compare October 18, 2024 19:10
@fpetrini15 fpetrini15 requested a review from GuanLuo October 21, 2024 16:40
GuanLuo
GuanLuo previously approved these changes Oct 21, 2024
src/shared_library.h Outdated Show resolved Hide resolved
Comment on lines +327 to +332
std::wstring path_after;
DWORD len = GetEnvironmentVariableW(PATH.c_str(), NULL, 0);
if (len > 0) {
path_after.resize(len);
GetEnvironmentVariableW(PATH.c_str(), &path_after[0], len);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need to change in this PR, but looks like this pattern of

  • check size of env var
  • resize local string
  • write env var to string

is used at least 3 times - probably good to be a helper function in the future

@fpetrini15 fpetrini15 requested a review from GuanLuo October 22, 2024 20:21
@fpetrini15 fpetrini15 merged commit 4969a5b into main Oct 23, 2024
1 check passed
@fpetrini15 fpetrini15 deleted the fpetrini-additional-dep-dir branch October 23, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feat A new feature
Development

Successfully merging this pull request may close these issues.

5 participants