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

Add IsVirtualDeivce() and refactor #6726

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

yeounoh
Copy link
Contributor

@yeounoh yeounoh commented Mar 12, 2024

This addresses the following to support #6322

  • add IsVirtualDeivce()
  • Un-const tensors argument to XLAGraphExecutor::Compile method, since they can be updated during auto-sharding paass.
  • refactor to remove redundant codes

Comment on lines -44 to +42
data_ = data;
data_.reset(data.get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference between these two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better memory management, this is refactoring to reuse the existing data_ to hold the new data.

@@ -346,7 +346,7 @@ class XLAGraphExecutor : public torch::lazy::LazyGraphExecutor {
std::vector<size_t> SetBufferDonors(LoweringContext* lowering_ctx);

// We don't use upstream Compile to have BuildInputOutputAliases.
CompilationResult Compile(const std::vector<XLATensorPtr>& tensors,
CompilationResult Compile(std::vector<XLATensorPtr>& tensors,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see how this will become problematic for the dynamo, since in dynamo we will first dry run the compilation and don't execute the compiled program. Please leave a TODO somewhere for the dynamo integration with auto-sharding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense I will also add a section in the design doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// We don't use upstream Compile to have BuildInputOutputAliases.
  // TODO(yeounoh) auto-sharding can change tensors shardings, which needs to be
  // accounted for in Dynamo integration.
  CompilationResult Compile(std::vector<XLATensorPtr>& tensors,
                            absl::Span<const std::string> devices,
                            const SyncTensorCollection& coll,
                            PostOrderData* po_data,
                            const std::vector<torch::lazy::Value>& ir_values);

sharding_specs.push_back("");
}
sharding_specs.push_back(
GetXLAShardingSpec(bridge::GetXlaTensor(tensor)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

what will GetXLAShardingSpec return when sharding_spec is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"" empty string as expected.

@yeounoh yeounoh force-pushed the add_virtual_deivce_utils branch from 11cc726 to 381077a Compare March 13, 2024 00:30
@yeounoh yeounoh force-pushed the add_virtual_deivce_utils branch from 381077a to b21f260 Compare March 13, 2024 05:21
@yeounoh yeounoh merged commit 7d89913 into master Mar 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants