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

Fix (graph/equalize): cleanup and device management #840

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

Giuseppe5
Copy link
Collaborator

No description provided.

@Giuseppe5 Giuseppe5 force-pushed the eq_fix branch 2 times, most recently from 5f6bdda to e702d87 Compare February 11, 2024 15:13
@Giuseppe5 Giuseppe5 requested a review from nickfraser February 11, 2024 15:15
@Giuseppe5 Giuseppe5 added the next release PRs which should be merged for the next release label Feb 11, 2024
@nickfraser
Copy link
Collaborator

Does it make sense to explicitly specify 'cpu' here in all cases? I assume this would have equalization go slower in cases where everything would fit on the GPU. What you had originally seems to make perfect sense - to keep everything in the model device memory.

Perhaps we should consider a context manager (*gasp!* Yes, I know...) which can be deployed, which overrides this original functionality to provide a "memory efficient" version.

In any case, I think such functionality is not vital and I'm happy for this to be merged in the meantime...

@Giuseppe5
Copy link
Collaborator Author

These operations are not compute heavy so I'm not expecting a considerable slowdown. I guess the issue is more that we're copying data to/from cpu several times which is not ideal.

The problem with the current implementation is that we do not account for the possibility of mismatched devices (e.g., GPU 0 and 1). We could try to be smarter and pick one device instead of always falling back on CPU, but again the main issue is data movement, not compute, so that wouldn't solve much.

Overall I'm happy with this solution since it's a good trade-off of complexity vs benefits.

@Giuseppe5 Giuseppe5 requested review from nickfraser and removed request for nickfraser February 12, 2024 11:56
@Giuseppe5 Giuseppe5 merged commit 930c2fd into Xilinx:dev Feb 12, 2024
345 of 347 checks passed
@Giuseppe5 Giuseppe5 deleted the eq_fix branch February 12, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release PRs which should be merged for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants