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

Storing system descriptor #265

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

sdjukicTT
Copy link
Contributor

@sdjukicTT sdjukicTT commented Feb 19, 2025

Ticket

#201

Problem description

Default system descriptor used in TTIRToTTNNBackendPipeline can't be used for multichip, actual current system descriptor is needed.

What's changed

We store the system descriptor and then set options.systemDescPath.

Checklist

  • New/Existing tests provide coverage for changes

Copy link

github-actions bot commented Feb 19, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-XLA Tests603 ran432 passed171 skipped0 failed
TestResult
No test annotations available

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.57%. Comparing base (0a01029) to head (0643634).
Report is 32 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/common/module_builder.cc 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #265      +/-   ##
==========================================
+ Coverage   78.06%   78.57%   +0.50%     
==========================================
  Files          21       21              
  Lines        1044     1008      -36     
==========================================
- Hits          815      792      -23     
+ Misses        229      216      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sdjukicTT sdjukicTT force-pushed the sdjukic/store-system-desc branch from 90d1d7f to e1cd701 Compare February 21, 2025 19:15
Comment on lines +198 to +199
mlir::OwningOpRef<mlir::ModuleOp> &mlir_module,
const std::string &system_descriptor_path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Input-only arguments should be ordered before the output or in-out arguments, see more here: https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs


system_descriptor_ = system_desc;
system_descriptor_.store(cached_system_descriptor_path_.data());
Copy link
Contributor

Choose a reason for hiding this comment

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

Check after this if std::filesystem::exists(cached_system_descriptor_path_) and if it doesn't, do:

DLOG_F(ERROR, "Failed to store the system descriptor to the disk using path: %s",
    cached_system_descriptor_path_.c_str());
return tt_pjrt_status::kInternal;

DLOG_F(LOG_DEBUG, "ClientInstance::ClientInstance");
module_builder_ = std::make_unique<ModuleBuilder>();
cached_system_descriptor_path_ =
std::filesystem::temp_directory_path().concat(
"/tt_pjrt_system_descriptor");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a TODO comment here explaining that this name would need to be unique to avoid clashes between multiple clients, but since we plan soon to remove the need for storing the descriptor to the disk we are leaving it simple like this until then (or until it causes problems).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants