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

CompatHelper: bump compat for "ChainRulesCore" to "1" #344

Merged

Conversation

github-actions[bot]
Copy link
Contributor

This pull request changes the compat entry for the ChainRulesCore package from 0.9.44, 0.10 to 0.9.44, 0.10, 1.

This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request.

@theogf theogf force-pushed the compathelper/new_version/2021-07-24-00-38-28-620-1792200906 branch from 2d39cc0 to 95f9816 Compare July 24, 2021 00:38
st-- added 2 commits August 9, 2021 15:08
…tions.jl into compathelper/new_version/2021-07-24-00-38-28-620-1792200906
@willtebbutt
Copy link
Member

@st-- the Others tests were failing on Julia 1.6 because the InplaceableThunks now accept their arguments in the opposite other from before. I've restricted CRC to version 1 as a consequence.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@willtebbutt
Copy link
Member

Verified locally that JuliaDiff/ChainRules.jl#496 fixes the problems in the Transform tests.

We need to make a decision about dropping CI for 1.3. I don't really want to have to deal with maintaining such an old version, given we have limited resources.

@devmotion
Copy link
Member

Sorry, it's not clear to me why 1.3 is a problem since ChainRules seems to be compatible with Julia 1. It seems there's no compatible version of Zygote available even though Zygote is also compatible with Julia 1.3?

@willtebbutt
Copy link
Member

Yeah, it's also not really clear to me. The dependencies look like they're a mess tbh. It looks like Flux needs 1.6 now though, so perhaps that's the souce of the problem?

@devmotion
Copy link
Member

I see. Maybe we should just get rid of Flux in our tests? It seems it is mainly used to check the parameters but that could be done with Functors more directly.

@willtebbutt
Copy link
Member

willtebbutt commented Aug 13, 2021

New version of ChainRules is available, which will hopefully fix (at least one of) the problems that we were facing. I'll go ahead and merge if everything looks okay.

edit: I forgot about the 1.3 problems -- I'll take a stab at getting that to work.

@willtebbutt
Copy link
Member

Unfortunately the NeuralKernelNetwork tests won't work without Flux because they explicitly require the Chain struct from it.

I don't believe there's a way to straightforwardly write the code in a manner which depends on another package, so I'm not entirely sure what to do beyond dropping 1.3 or weakening some of our existing tests on all versions :(

@devmotion
Copy link
Member

Unfortunately the NeuralKernelNetwork tests won't work without Flux because they explicitly require the Chain struct from it.

Do we need to test Chain explicitly? It seems it is only used in

# Apply standard test suite.
TestUtils.test_interface(nkn, Float64)
and we don't make use of all the sophisticated implementation and features of Chain?

@willtebbutt
Copy link
Member

Hmmm you're probably right. I'll give it a go and see what it looks like.

@willtebbutt
Copy link
Member

I think there's a reasonable chance that 1.3 tests will pass now.

It's a bit of a hack to ensure that the current params tests continue to pass (I literally copy + pasted code from Flux). If the tests pass I'll look in to changing the tests so we don't have this copy + pasted code.

willtebbutt and others added 3 commits August 13, 2021 17:01
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@willtebbutt
Copy link
Member

Nearly there, just some AD issues on 1.3 with BaseKernels.

@willtebbutt
Copy link
Member

Tests now appear to pass, but I've not been able to determine a straightforward way to replace the Flux code. Possibly this is something that @theogf could look into upon returning from his holiday.

In the mean time, @devmotion is there anything else that needs doing before we merge this, or can I go ahead and merge?

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I think it's beneficial if we don't have to depend on Flux in our tests. It reminds me of how annoying the PPL tests in AbstractGPs were and how they messed with other dependencies before they moved to a separate environment. Maybe something similar could be done here, if at some point it seems important to add tests with Flux (I don't think it's useful currently).

@willtebbutt willtebbutt merged commit b8ae35c into master Aug 15, 2021
@willtebbutt willtebbutt deleted the compathelper/new_version/2021-07-24-00-38-28-620-1792200906 branch August 15, 2021 13:07
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