fix: Respect cc_model_filenames in Python backend auto-fill#489
fix: Respect cc_model_filenames in Python backend auto-fill#489nightflight-dk wants to merge 12 commits into
Conversation
Co-authored-by: Yingge He <157551214+yinggeh@users.noreply.github.com>
ManagedIdentityCredentialOptions does not have a ClientId field. Use the constructor overload that accepts clientId as a string parameter instead. Also consolidate identity includes to the top-level azure/identity.hpp header and remove WIP comment.
cfae5e6 to
5f7ac3b
Compare
|
@whoisj this helps provide cc dependent implementation part of the same repo |
|
LGTM. I'll run it through our internal CI lab. @nightflight-dk, could you please address the pre-commit issues? Thanks. CI Pipeline ID: 49121709 you can always run pre-commit locally via
|
5f7ac3b to
31c39bb
Compare
* Fix RequestTracker counter mismatch in ScheduleSteps with parallel failures
31c39bb to
0fc4a6d
Compare
…spected-by-py-backend
There was a problem hiding this comment.
Pull request overview
Fixes Python backend auto-fill so default_model_filename is only defaulted to model.py when both default_model_filename and cc_model_filenames are empty, preserving user-provided compute-capability filename mappings.
Changes:
- Update
AutoCompleteBackendFieldsto skip defaultingdefault_model_filenamewhencc_model_filenamesis populated. - Add a dedicated unit test binary covering Python backend defaulting / non-defaulting behavior.
- Ignore
/build_testoutput directory.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/model_config_utils.cc |
Adjusts Python backend auto-fill condition to respect cc_model_filenames. |
src/test/model_config_utils_test.cc |
Adds unit tests validating the new auto-fill behavior and preservation rules. |
src/test/CMakeLists.txt |
Adds model_config_utils_test target and links required dependencies. |
.gitignore |
Ignores build_test directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string cmd = "rm -rf " + root_path_; | ||
| (void)system(cmd.c_str()); |
There was a problem hiding this comment.
TempModelDir::~TempModelDir uses system("rm -rf ...") for cleanup. This is non-portable (breaks on Windows), can behave incorrectly if the path contains spaces/shell metacharacters, and system may not be declared here (no include), causing a compile failure in C++ builds. Prefer using the existing filesystem helper (e.g., triton::core::DeletePath(root_path_)), and optionally assert/expect the returned Status in teardown/cleanup.
| std::string cmd = "rm -rf " + root_path_; | |
| (void)system(cmd.c_str()); | |
| auto status = tc::DeletePath(root_path_); | |
| EXPECT_TRUE(status.IsOk()) << status.AsString(); |
| std::string version_dir = tc::JoinPath({root_path_, version}); | ||
| mkdir(version_dir.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH); | ||
| if (!filename.empty()) { | ||
| std::ofstream f(tc::JoinPath({version_dir, filename})); | ||
| f << "# placeholder"; | ||
| } |
There was a problem hiding this comment.
Test setup ignores failures from mkdir(...) and from writing the placeholder file (stream open/write errors). If either step fails, the test may pass/fail for the wrong reason. Please check the return value of mkdir (and consider handling EEXIST), and validate the std::ofstream state (or use triton::core::MakeDirectory / WriteTextFile which return a Status).
| EXPECT_EQ(config.default_model_filename(), "") | ||
| << "default_model_filename should remain empty when cc_model_filenames " | ||
| "is set"; |
There was a problem hiding this comment.
In this case, which python file backend should load if not "gpu"?
fix: Respect
cc_model_filenamesin Python backend auto-fillProblem
When
backendis set to"python"anddefault_model_filenameis left empty,AutoCompleteBackendFieldsunconditionally setsdefault_model_filenameto"model.py". This ignores anycc_model_filenamesmappings the user may have configured to use a custom model filename (e.g.,custom_model.pyper device).This causes the Python backend stub to always look for
model.py, even whencc_model_filenamesexplicitly maps to a different file.Fix
In
model_config_utils.cc, the auto-fill ofdefault_model_filenamenow checks thatcc_model_filenamesis also empty before defaulting to"model.py":