Skip to content

[WIP] Allow turning off allowscalar copies for yacusolver - #270

Draft
kshyatt wants to merge 1 commit into
mainfrom
ksh/danger_mode
Draft

[WIP] Allow turning off allowscalar copies for yacusolver#270
kshyatt wants to merge 1 commit into
mainfrom
ksh/danger_mode

Conversation

@kshyatt

@kshyatt kshyatt commented Aug 17, 2026

Copy link
Copy Markdown
Member

The idea of this PR is to make the checks involving @allowscalar optional. These copies are extremely expensive and block the device, so by (optionally) disabling them, we should be able to make better use of the GPU.

So far I've just done the CUSOLVER ones, but I can also do ROCSOLVER if we like the idea. I think it would make sense to have a settable flag that turns the checks on and off so that users can control it, or a CheckedCUSOLVER driver. Anyone have thoughts on that angle?

@kshyatt
kshyatt requested review from Jutho and lkdvos August 17, 2026 08:23

@lkdvos lkdvos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Definitely looks reasonable to me, although it would be nice to get a sense of how bad the performance is to try and decide if we want to turn this on or off by default.

Is this specifically for the case where we apply the factorizations to a bunch of blocks and don't want to sync inbetween? There might also be a case for writing a batched version that tries to only sync at the end or something similar

@kshyatt

kshyatt commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Is this specifically for the case where we apply the factorizations to a bunch of blocks and don't want to sync inbetween?

Indeed it helps with this but not only here, another example is calc_convergence in PEPSKit where we're computing svd_vals 8 times (4 corners, 4 edges).

@lkdvos

lkdvos commented Aug 17, 2026

Copy link
Copy Markdown
Member

Yeah fair enough, I definitely think the keyword argument is a good solution, just slightly on the fence about disabling checks by default. 🤷 it's a bit annoying to think that PEPSKit would run without these and then if at some point something doesn't converge it just silently goes haywire, and then you have to do a full rerun with checks on to figure out what happened, but I guess that's what it has to be

@kshyatt

kshyatt commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

I agree, that's why I think in fact this should be settable at __init__ or with an env var. But what to call it?

@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ext/MatrixAlgebraKitCUDAExt/yacusolver.jl 36.36% 14 Missing ⚠️
Files with missing lines Coverage Δ
ext/MatrixAlgebraKitCUDAExt/yacusolver.jl 90.94% <36.36%> (-5.19%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite 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.

2 participants