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

Clean up and fix tests #125

Merged
merged 10 commits into from
Jan 23, 2024
Merged

Clean up and fix tests #125

merged 10 commits into from
Jan 23, 2024

Conversation

simsurace
Copy link
Member

@simsurace simsurace commented Jan 10, 2024

Checked out the registry at the time of the v0.6.5 release and then set the compat upper bounds to the resolved dependencies.

Supersedes #124

@simsurace simsurace changed the title Try to reproduce a working set of dependencies Clean up and fix tests Jan 10, 2024
@coveralls
Copy link

coveralls commented Jan 10, 2024

Pull Request Test Coverage Report for Build 7624664125

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on freeze-deps at 87.549%

Totals Coverage Status
Change from base Build 7303152969: 87.5%
Covered Lines: 1343
Relevant Lines: 1534

💛 - Coveralls

@simsurace
Copy link
Member Author

Yay, this works. I will include the other changes I made in #124 to clean things up.

This reverts commit 5b3cc7d.
@willtebbutt
Copy link
Member

This is fantastic. Would you mind checking to see whether all of the upper bounds are actually necessary? I can definitely imagine that the Zygote / ChainRules ones are, but it seems surprising to be that it's necessary for e.g. FillArrays.

@simsurace
Copy link
Member Author

simsurace commented Jan 20, 2024

That's what I was trying to avoid ☺️, I have spent way too much time trying to find such combinations, so that's why I went with the historical registry approach.I think some of the failures are due to JuliaArrays/FillArrays.jl#271. There was a long Slack thread you maybe did not see, I forwarded it to you on Slack.

@willtebbutt
Copy link
Member

willtebbutt commented Jan 22, 2024

Ahh okay. No, I didn't see that thread. Could we at least try un-pinning AbstractGPs and KernelFunctions? I would be completely amazed if non-breaking changes in them actually break AD in TemporalGPs.

@simsurace
Copy link
Member Author

Let's try

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@simsurace
Copy link
Member Author

This works. Let's then try removing some of the others.

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@simsurace
Copy link
Member Author

Ok, now I'm feeling bold. Let me push it a bit further.

Project.toml Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
@simsurace
Copy link
Member Author

@willtebbutt ok, this is more of a minimal set of pinnings that works.
Did you have any chance to look at the Slack thread? We might as well look at resolving the underlying issues first, but I don't know where to start.

@willtebbutt
Copy link
Member

I've had a look at the slack thread, but I'm not going to have a chance to dig into this for a while. I'm happy to approve this PR though

@simsurace
Copy link
Member Author

Thanks. But I think if we merge this right now, perhaps we should put a note at the top of the README, that this package is currently pinning some AD deps and might hold back environments or cause conflicts.

@willtebbutt
Copy link
Member

willtebbutt commented Jan 23, 2024

Another idea:

  1. release this patch (0.6.7)
  2. immediately make another patch release in which we unpin everything (0.6.8)
  3. add a note to the readme saying that version 0.6.7 has pinned dependencies, and we're confident that everything should work on that version, so if something if the current version isn't working for you, please try that version out?

@simsurace simsurace marked this pull request as ready for review January 23, 2024 10:55
@simsurace simsurace merged commit 6023595 into master Jan 23, 2024
12 checks passed
@simsurace simsurace deleted the freeze-deps branch January 23, 2024 10:56
@simsurace simsurace mentioned this pull request Jan 23, 2024
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.

None yet

3 participants