Skip to content

Zygote ext #218

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

Merged
merged 11 commits into from
Mar 12, 2023
Merged

Zygote ext #218

merged 11 commits into from
Mar 12, 2023

Conversation

vpuri3
Copy link
Contributor

@vpuri3 vpuri3 commented Mar 8, 2023

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 8, 2023

running CI with dummy change since tests were failing locally.

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Patch coverage: 10.52% and project coverage change: +0.10 🎉

Comparison is base (c2bbf98) 0.21% compared to head (21a9a1e) 0.31%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #218      +/-   ##
=========================================
+ Coverage    0.21%   0.31%   +0.10%     
=========================================
  Files          15      14       -1     
  Lines         936     940       +4     
=========================================
+ Hits            2       3       +1     
- Misses        934     937       +3     
Impacted Files Coverage Δ
ext/SparseDiffToolsZygote.jl 0.00% <0.00%> (ø)
src/SparseDiffTools.jl 37.50% <50.00%> (-2.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 8, 2023

@ChrisRackauckas looks like SparseDiffTools needs more comprehensive testing. I am getting arrayinerface errors with 1.9 on my personal machine that are not captured on CI.

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 11, 2023

Looks like package extension don't honor export statements. Rather, it is just

julia> versioninfo()
Julia Version 1.9.0-rc1
Commit 3b2e0d8fbc1 (2023-03-07 07:51 UTC)

(a) pkg> st                                                                          
Status `~/.julia/dev/SparseDiffTools/a/Project.toml`
  [47a9eef4] SparseDiffTools v2.0.0 `..`
  [e88e6eb3] Zygote v0.6.56                                                          
                                          
julia> using SparseDiffTools, Zygote  
[ Info: Precompiling SparseDiffToolsZygote [25f82377-10ae-5fb8-bc0d-27fd31de4285]
                                          
julia> m = Base.get_extension(SparseDiffTools, :SparseDiffToolsZygote)
SparseDiffToolsZygote 

julia> m.ZygoteVecJac
ZygoteVecJac (generic function with 1 method)

julia> ZygoteVecJac
ERROR: UndefVarError: `ZygoteVecJac` not defined

julia> SparseDiffTools.ZygoteVecJac
ERROR: UndefVarError: `ZygoteVecJac` not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)

So I think we should define and export these symbols in the main package, and write method definitions in the extension.

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 12, 2023

@ChrisRackauckas can you merge #220 so we can test for 1x9 here?

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 12, 2023

rerunning CI

@vpuri3 vpuri3 closed this Mar 12, 2023
@vpuri3 vpuri3 reopened this Mar 12, 2023
@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 12, 2023

@ChrisRackauckas good to go

@ChrisRackauckas ChrisRackauckas merged commit 079a63b into JuliaDiff:master Mar 12, 2023
@vpuri3 vpuri3 deleted the zygote-ext branch March 12, 2023 19:25
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