Update dependency lower bounds and test with CI #120
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
=======================================
Coverage 96.65% 96.65%
=======================================
Files 11 11
Lines 867 867
=======================================
Hits 838 838
Misses 29 29 ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 7899028199Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
IMO the downgrade CI is a bit restrictive - it's not a problem if Pkg can't resolve all lower bounds at the same time, as long as for each lowest bound the oldest version of packages that it can be resolved with passes the tests, is it?
Nevertheless, I think the checks are helpful. The lower bounds of docs/Project.toml are not tested though, are they (just curious due to the update of docs/Project.toml)?
Agreed, it's just that this would require too many checks. IMO the downgrade approach makes sense if the changes it requires are not too strict. If there's a dependency (e.g. SpecialFunctions) that we really don't want to downgrade, we could of course tell the action to skip it. But we wouldn't be able to test older versions of SpecialFunctions without lowering the lower bounds of Distributions and StatsFuns, and at that point, we wouldn't be able to simultaneously test these lower bounds.
No, and they would require some updates. The only change I made there was to match the lower bounds used for the package/test, since I think it generally makes sense to keep these consistent. |
|
Locally everything passes if I downgrade both I'll troubleshoot. |
EvoTrees recent versions use a method of `Tables.DataAPI.nrow` only defined in Tables v1.11.
|
Regarding test/Project.toml - could we switch back to an extras section in Project.toml? IIRC test/Project.toml was at some point useful to work around issues with test dependencies and the Pkg reaolver but maybe these problems are fixed? |
Done! I checked locally, and to build docs with lower bounds, we would additionally need to change the following in
And in
MLJBase is the only important one of these, and since v1 is out, it doesn't seem like a problem to make the update. But we also won't be building the docs with downgraded versions yet. Perhaps in the future we could do it as a test when the github action supports it. So I'm neutral about making these changes. |
|
I think I tend to view the doc environment as a "dynamic" environment rather than a package: version bounds are useful to avoid breakages due to a dependency (and CompatHelper notifies us about breaking releases) but we're only interested in building docs with the latest compatible releases of the dependencies and don't want to rebuild the docs with older versions. Hence I think we could generally remove any older semver-breaking versions in the docs, and we could even update it to the latest versions only (but IMO the latter is not neccessary). |
|
IIUC, that would mean at least bumping the docs versions as suggested in #120 (comment), but leaving |
This avoids an error that only manifests on ~0.14.7 when using a single thread
As suggested by https://www.stochasticlifestyle.com/semantic-versioning-semver-is-flawed-and-downgrade-ci-is-required-to-fix-it/, this PR adds a CI step that downgrades all dependencies to their lower bounds before running CI.
This required bumping some lower bounds that were incompatible with other dependencies. Most of these are within the same version series by semver, the exception being that we drop support for SpecialFunctions
0.8, 0.9, 0.10. This should be fine though, since the current version is2.x.x.