Skip to content

Conversation

@Aki-07
Copy link

@Aki-07 Aki-07 commented Nov 29, 2025

What does this PR do?

Fixes #11966

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@sayakpaul

@Aki-07 Aki-07 force-pushed the feature/group-offload-pinning branch from 7e50d90 to 3b3813d Compare November 29, 2025 14:03
@sayakpaul
Copy link
Member

Thanks for your PR. However, it's being worked on in #12721.

@sayakpaul
Copy link
Member

sayakpaul commented Dec 9, 2025

Could we resolve conflicts so that it's a bit easier to review? Seems like there's some overlap from #12692.

@Aki-07 Aki-07 force-pushed the feature/group-offload-pinning branch from 6d96002 to 33d8b52 Compare December 10, 2025 06:06
@Aki-07
Copy link
Author

Aki-07 commented Dec 10, 2025

Done! Rebased on latest main and resolved conflicts with #12692. Should be much cleaner to review now.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Some initial comments.

Comment on lines 310 to 318
should_synchronize = (
not self.group.onload_self and self.group.stream is not None and not should_onload_next_group
)
Copy link
Member

Choose a reason for hiding this comment

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

What if non_blocking=True?

Copy link
Author

Choose a reason for hiding this comment

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

Even with non_blocking=True, if a previous group onloaded this one on a side stream, we need a sync before the default stream uses the weights or we risk reading half-copied tensors. I’ve limited the sync to the record_stream=False case, when record_stream=True the tensors are tied to the consumer stream so we can safely skip the sync.

@bconstantine
Copy link

Thank you for the initial comment! We are working on the solutions right now

@Aki-07 Aki-07 force-pushed the feature/group-offload-pinning branch from 6f5887e to 1194a83 Compare December 11, 2025 01:53
@sayakpaul
Copy link
Member

@bot /style

@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2025

Style bot fixed some files and pushed the changes.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

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.

How about forcing the first and last block on device when groupoffloading is used?

4 participants