Skip to content

Fix check of whether to copy arg for Enzyme#239

Merged
kshyatt merged 2 commits into
mainfrom
ksh/enzfix
May 28, 2026
Merged

Fix check of whether to copy arg for Enzyme#239
kshyatt merged 2 commits into
mainfrom
ksh/enzfix

Conversation

@kshyatt

@kshyatt kshyatt commented May 27, 2026

Copy link
Copy Markdown
Member

No description provided.

@kshyatt kshyatt requested a review from lkdvos May 27, 2026 12:15
@kshyatt

kshyatt commented May 27, 2026

Copy link
Copy Markdown
Member Author

Verified this fixed the enzyme tests locally. I'll try to work out where the problem is for newer versions and fix it but in the meantime this should help CI.

@codecov

codecov Bot commented May 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ixAlgebraKitEnzymeExt/MatrixAlgebraKitEnzymeExt.jl 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
...ixAlgebraKitEnzymeExt/MatrixAlgebraKitEnzymeExt.jl 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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.

I understand this fixes the CI, but I think effectively this means that if you for whatever reason have a newer version of Enzyme, it will just revert to an older version of MatrixAlgebraKit that is compatible with that. I'm not sure what the recommended way around this is, does this mean we should just kind of wait for Enzyme to unbreak?

@kshyatt

kshyatt commented May 27, 2026

Copy link
Copy Markdown
Member Author

I also opened an issue over there and am trying to figure out the cause at the moment. I was mostly thinking we could effectively revert this once the problem is fixed but for now it won't spuriously break CI for other people 🤷

@kshyatt

kshyatt commented May 27, 2026

Copy link
Copy Markdown
Member Author

But maybe it's unnecessary as I think I found the problem...

@kshyatt kshyatt changed the title Force usage of working Enzyme Fix check of whether to copy arg for Enzyme May 27, 2026
@kshyatt

kshyatt commented May 27, 2026

Copy link
Copy Markdown
Member Author

OK, it seems on more recent versions of Enzyme, this arg.val === ret check (which we originally added for the case when arg = (nothing, nothing)) varies depending on the Enzyme version. I think it doesn't make sense to check this alongside the one for whether A is present in arg so I've modified that and it seems to make everyone happy.

@lkdvos

lkdvos commented May 27, 2026

Copy link
Copy Markdown
Member

I have very little feeling with what is the correct way of doing this since I don't know the Enzyme details, this definitely looks reasonable. Is there any benefit of factoring that out into a separate function, which is then at least centralized?

@kshyatt

kshyatt commented May 28, 2026

Copy link
Copy Markdown
Member Author

Is there any benefit of factoring that out into a separate function, which is then at least centralized?

There may be but I think there is a bunch of refactoring of these rules I'd like to do in a separate PR (e.g. handle all the code duplication in the truncated rules)

@kshyatt kshyatt merged commit 08eaf37 into main May 28, 2026
33 of 36 checks passed
@kshyatt kshyatt deleted the ksh/enzfix branch May 28, 2026 08:20
@lkdvos lkdvos mentioned this pull request Jun 2, 2026
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