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: hotfix Zygote backend of DifferentiationInterface #404

Closed
wants to merge 1 commit into from

Conversation

MilesCranmer
Copy link
Owner

@MilesCranmer MilesCranmer commented Jan 31, 2025

DifferentiationInterface introduced a change in behavior where the Zygote backend returning nothing is now an error rather than nothing; this implements a hotfix. It's not a great solution but I'd prefer this over another extension.

FYI @gdalle I think the ZygoteNothingError should have been introduced in a breaking release as normally it's common to handle nothing explicitly. Though I do think it's the right behavior though overall. Actually changed my mind, I think the nothing should just be returned.

Also would be nice to have ZygoteNothingError defined inside the package itself. If not wanting to put it there, perhaps you could have some trait like:

nothing_error_type(_) = Nothing


# in Zygote extension:
nothing_error_type(::AutoZygote) = ZygoteNothingError

then I can simply call e isa nothing_error_type(backend).

Or maybe have an abstract type of error for when a gradient isn't defined?

Copy link
Contributor

Benchmark Results

master 146f49d... master/146f49ddd8031f...
search/multithreading 17 ± 0.97 s 17.2 ± 1.1 s 0.989
search/serial 29.6 ± 0.66 s 29.7 ± 0.85 s 0.994
utils/best_of_sample 1.79 ± 1 μs 1.59 ± 0.52 μs 1.13
utils/check_constraints_x10 12 ± 3.3 μs 12.1 ± 3.4 μs 0.992
utils/compute_complexity_x10/Float64 2.18 ± 1.5 μs 2.18 ± 1.6 μs 1
utils/compute_complexity_x10/Int64 2.11 ± 0.1 μs 2.1 ± 1.6 μs 1
utils/compute_complexity_x10/nothing 1.52 ± 1.5 μs 1.47 ± 1.6 μs 1.03
utils/insert_random_op_x10 6.09 ± 2.7 μs 5.74 ± 1.9 μs 1.06
utils/next_generation_x100 0.405 ± 0.087 ms 0.359 ± 0.1 ms 1.13
utils/optimize_constants_x10 0.037 ± 0.0087 s 0.036 ± 0.0083 s 1.03
utils/randomly_rotate_tree_x10 5.37 ± 0.7 μs 5.43 ± 0.73 μs 0.989
time_to_load 1.82 ± 0.043 s 1.81 ± 0.019 s 1

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13081620250

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 94.79%

Totals Coverage Status
Change from base Build 12680128577: 0.004%
Covered Lines: 3166
Relevant Lines: 3340

💛 - Coveralls

@MilesCranmer
Copy link
Owner Author

Fixed at the source with JuliaDiff/DifferentiationInterface.jl#714

@MilesCranmer MilesCranmer deleted the fix-zygote-backend branch February 1, 2025 20:49
@gdalle
Copy link

gdalle commented Feb 1, 2025

JuliaRegistries/General#124177

@MilesCranmer
Copy link
Owner Author

Thanks!

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.

3 participants