Use destructure from Optimisers.jl
#1901
Conversation
|
Failures in Metalhead look like this, I don't see them on its master, not sure if they are caused by Flux: Failures in FastAI and OperatorLearning are about |
|
Metalhead failures are unrelated to this PR: FluxML/Metalhead.jl#127 |
Project.toml
Outdated
| version = "0.13.0-DEV" | ||
| version = "0.12.99" |
There was a problem hiding this comment.
Shouldn't forget to change this back! Just trying to ensure downstream tests do run.
| @@ -4,8 +4,6 @@ using Zygote: IdSet | |||
| import Functors: Functors, @functor, functor, fmap, isleaf | |||
| using SparseArrays: AbstractSparseArray | |||
|
|
|||
| trainable(m) = functor(m)[1] | |||
There was a problem hiding this comment.
This caused:
julia> using Flux
[ Info: Precompiling Flux [587475ba-b771-5e3f-ad9e-33799f191a9c]
WARNING: Method definition trainable(Any) in module Optimisers at /Users/me/.julia/dev/Optimisers/src/interface.jl:69 overwritten in module Flux at /Users/me/.julia/dev/Flux/src/functor.jl:7.
** incremental compilation may be fatally broken for this module **
|
@gabrevaya this might solve the example here: #1353 (comment) But what it |
Codecov Report
@@ Coverage Diff @@
## master #1901 +/- ##
==========================================
- Coverage 86.57% 86.42% -0.15%
==========================================
Files 18 18
Lines 1445 1422 -23
==========================================
- Hits 1251 1229 -22
+ Misses 194 193 -1
Continue to review full report at Codecov.
|
.github/workflows/Downstream.yml
Outdated
| @@ -27,7 +28,7 @@ jobs: | |||
| - {user: SciML, repo: DiffEqFlux.jl, group: Layers} | |||
| - {user: SciML, repo: NeuralPDE.jl, group: NNPDE} | |||
| - {user: SciML, repo: OperatorLearning.jl, group: All} | |||
| if: contains(github.event.pull_request.labels.*.name, 'run downstream test') | |||
| # if: contains(github.event.pull_request.labels.*.name, 'run downstream test') | |||
There was a problem hiding this comment.
This change should be removed before merging.
There was a problem hiding this comment.
You are thinking to save CI time?
Only the NNPDE one is long, 45 minutes. So I think downstream tests are quicker in total than the main tests. I was wondering whether it would be simplest to always run them, but not sure.
Maybe ideal would be to set things up that the basic test on Julia 1 must pass before any others are run. I think CUDA.jl has something like that. But not sure how easy it is to set up.
There was a problem hiding this comment.
Even if they don't take more time it's still wasted energy in most cases. Adding a label to sensitive PRs seems an effortless compromise.
|
As an extra layer of caution we could rerun downstream tests for NeuralPDE and DiffEqFlux with group All |
|
Tests for DiffEqFlux.jl don't run, sounds like there is a manifest checked in which wants NNlib 0.7, but not sure where: Tests for NeuralPDE.jl fail, not sure why. Maybe @ChrisRackauckas knows? Both packages use |
|
We don't intentionally restrict NNLib to v0.7 anywhere that I know of. I'm rather surprised that it's not using v0.8 given the compats I see on the repos. |
|
Test deps were done incorrectly. A new patch fixes that. |
|
Great. Any idea why NeuralPDE tests fail, or how to isolate one? |
|
Re-run the NeuralPDE one and see if you get the same result or if it was just a bad initial condition? It's not failing other downstream tests |
|
@yuehhua and @avik-pal there was another breaking change in GeometricFlux? @mcabbott the NeuralPDE failure looks real, as in, that's one of the few cases that uses Chain instead of FastChain, and it just so happens to now be failing but I cannot reproduce it in the repo's CI. @zoemcc can maybe help isolate this? |
|
@ChrisRackauckas If there are breaking changes, I will tag minor release for GeometricFlux. Currently, the breaking changes in GeometricFlux will be the message-passing network part. Did something happened here? |
|
For this: For DiffEqFlux, current error looks unrelated but it's unclear to me whether https://github.com/SciML/DiffEqFlux.jl/blob/master/test/runtests.jl runs all tests after one failure, or gives up. |
|
Any luck isolating the NeuralPDE failure? (I think Chris meant to tag @zoemcc above, but version with a link won't notify.) |
|
Looks like it was a fluke 🎉 |
|
Note that 91d88fe reverts the version & downstream changes --- they now pass in 2 mins for packages which don't declare compatibility with Flux 0.13. Still easy to investigate locally by |
|
Ahh, I thought that was a rebase. Yeah sorry, it seems like I messed up the ping earlier. Feel free to merge, and if NeuralPDE has a failure then at least we know why. |
This deletes
destructurehere to use the function from Optimisers.jl, of the same name and almost the same behaviour, except that:But, of course, there are bugs, Flux's tests fail locally.Should also add more tests...Now includes tests from these issues:Fixes #1826
Fixes #1767
Fixes #1601
Fixes #1727
Fixes #1733, closes #1742 (replaces)