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 several minor fixes #32

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Add several minor fixes #32

merged 1 commit into from
Feb 10, 2025

Conversation

MC952-arch
Copy link
Collaborator

  • Invoke event block call in WorkFlagCX::wait func
  • Add plugin/torch/run_hetero.sh script for heterogeneous test
  • Reuse sync stream as a substitution of barrier op for better performance

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please refactor this file in a more modular way in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK

Comment on lines +141 to +144
if (!coalesced_)
{
event_->block(device_id_);
}
if (isBarrierOp_)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for this code working now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the reason for this code working now?

It seems that a sync stream call is required after flagcxHeteroSend/Recv ops. Such synchronization is added in all C2C algorithms for the moment. Here we adopt the same way as ProcessGroupNCCL does, blocking current stream on the flagcx stream.

Copy link
Collaborator

@aoyulong aoyulong left a comment

Choose a reason for hiding this comment

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

Something will be Improved in the future.

@aoyulong aoyulong merged commit 17cac39 into FlagOpen:main Feb 10, 2025
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.

2 participants