Skip to content

Revert "feat: support DuplicatedNoNeed as function annotation for Enzyme"#807

Merged
gdalle merged 1 commit intomainfrom
revert-805-gd/noneed
May 28, 2025
Merged

Revert "feat: support DuplicatedNoNeed as function annotation for Enzyme"#807
gdalle merged 1 commit intomainfrom
revert-805-gd/noneed

Conversation

@gdalle
Copy link
Copy Markdown
Member

@gdalle gdalle commented May 28, 2025

Reverts #805

@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.93%. Comparing base (70e91d7) to head (eaae10a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #807      +/-   ##
==========================================
- Coverage   97.93%   97.93%   -0.01%     
==========================================
  Files         128      128              
  Lines        7697     7693       -4     
==========================================
- Hits         7538     7534       -4     
  Misses        159      159              
Flag Coverage Δ
DI 98.81% <100.00%> (-0.01%) ⬇️
DIT 95.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gdalle gdalle merged commit f07b6ec into main May 28, 2025
50 checks passed
@gdalle gdalle deleted the revert-805-gd/noneed branch May 28, 2025 12:14
@wsmoses
Copy link
Copy Markdown

wsmoses commented May 30, 2025

why was this reverted?

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented May 30, 2025

Because the tests passed but I don't understand why. Despite the follow-up exchange on Discourse I still don't fully get what I'm allowed to do with DuplicatedNoNeed, nor can I think of a meaningful test case where the closure storage is write-only (what good does a write-only storage do?).
The test case I had in the DI test suite writes to closure storage and then reads back from it, but switching to function_annotation=DuplicatedNoNeed did not make it fail. I'm not sure why, so as a precaution I'm putting correctness over performance until I figure it out.

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented May 30, 2025

Essentially I don't want to release something where tests are supposed to fail but succeed and I don't know why.

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented May 30, 2025

The test case in question is here:

https://github.com/JuliaDiff/DifferentiationInterface.jl/blob/main/DifferentiationInterfaceTest/src/scenarios/modify.jl#L108-L164

I take a standard test function, and then I force both the input and the output to go through a buffer

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