Switch to Libtask 0.7

[rikhuijzer]

I'll also make a PR in the releases branch.

Oh wait, Hong already set the Libtask to 0.7 on the 0.3.5 release. I'll close this

[devmotion]

Probably we should change the Trace struct, otherwise the task field has an abstract type. This requires a breaking release though and additional changes in places like Vector{Trace}.

[coveralls]

Pull Request Test Coverage Report for Build 1875168839

Warning: 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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 61.033%

---

Totals
Change from base Build 1872975055:	0.0%
Covered Lines:	260
Relevant Lines:	426

---

💛 - Coveralls

[devmotion]

 Hong already set the Libtask to 0.7 on the 0.3.5 release.

We could fix this compat entry in the registry if we want to avoid the abstract fields.

[rikhuijzer]

We could fix this compat entry in the registry if we want to avoid the abstract fields.

I've now proposed to pin it at Turing (TuringLang/Turing.jl#1779).

Maybe it would actually be good to move AdvancedPS#master to Libtask 0.7 too to avoid possible confusion.

Sorry for me being all over the place here. I'm having zero experience with a staging environment and am having trouble to understand how things go here in this repo.

[codecov]

Codecov Report

Merging #45 (ab27c97) into master (88e9dad) will increase coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   61.03%   61.26%   +0.23%     
==========================================
  Files           6        6              
  Lines         426      426              
==========================================
+ Hits          260      261       +1     
+ Misses        166      165       -1     

Impacted Files	Coverage Δ
src/container.jl	94.70% <0.00%> (+0.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88e9dad...ab27c97. Read the comment docs.

[devmotion]

My point is independent of the AdvancedPS branches: It was a mistake to declare any version of AdvancedPS to be compatible with Libtask 0.7. AdvancedPS has to be updated properly first, e.g., by changing the Trace struct - but since this involves breaking changes it has to go into a breaking release. So IMO there should not be any 0.3.x release that it is declared compatible with Libtask 0.7.

[rikhuijzer]

So IMO there should not be any 0.3.x release that it is declared compatible with Libtask 0.7.

Just to be sure. You mean "shouldn't have" because 0.3.5 has Libtask 0.7 in the compat entry (https://github.com/TuringLang/AdvancedPS.jl/blob/releases/Project.toml).

[devmotion]

Yes. But that can be fixed in the registry.

[yebai]

You mean "shouldn't have" because 0.3.5 has Libtask 0.7 in the compat entry (https://github.com/TuringLang/AdvancedPS.jl/blob/releases/Project.toml).

It was motivated to fix the Libtask integration test. Since the releases branch is only meant for hotfixes, it might be ok to keep the compact entry there -- if it does not cause any harm. Otherwise, we will have to either

1. make a new release from AdvancedPS#master,
2. or, keep backporting new changes to AdvancedPS@releases once Turing requires [email protected]

[devmotion]

it might be ok to keep the compact entry there -- if it does not cause any harm.

The problem is that it's not a simple update - suddenly all nicely typed structs and containers have abstract fields or type parameters. IMO it would have been cleaner to make a 0.4 release with the Libtask 0.7 compat update and fix all these issues at the same time. I guess we could make a 0.4 release on the releases branch if we don't want to release the other changes on the master branch right now, and then update the master branch to 0.5. Alternatively, if the releases branch should only contain 0.3.x releases we could fork another releases-0.4 branch from it and add the compat update there.

[yebai]

suddenly all nicely typed structs and containers have abstract fields or type parameters.

I'm not sure I follow all the details here. The only change in #44 is replacing CTask with TapedTask. @devmotion can you clarify your point?

[devmotion]

Yes, in #44 only CTask is replaced with the alias TapedTask (and the field ctask was renamed to task which caused a hiccup in Turing). This is completely fine, the alias existed in Libtask 0.6.7-0.6.x so the compat entries did not have to be updated.

In Libtask 0.7, however, TapedTask is not a concrete type anymore since a type parameter was added. This is potentially breaking since, as in AdvancedPS, structs with a field of type TapedTask that was concretely typed now contain a field with an abstract type TapedTask. Even more importantly, it breaks dispatches on e.g. Vector{TapedTask} since this won't catch Vector{TapedTask{F}} for some F. Since at least one of these issues shows up in AdvancedPS (not sure about dispatches, haven't checked the code in detail) updating the Libtask compatibility 0.7 can have undesired (and potentially breaking) consequences. Therefore I think updating to Libtask 0.7 should be done more carefully and after considering and fixing these issues.

[yebai]

structs with a field of type TapedTask that was concretely typed now contain a lot with an abstract type TapedTask. Even more importantly, it breaks dispatches on e.g. Vector{TapedTask} since this won't catch Vector{TapedTask{F}} for some F.

Since we plan to make Libtask.TapeTask a concrete type soon, maybe we can leave these types as-is, just for now? It would take a lot of time to update AdvancedPS to catch up with intermediate changes in Libtask. That's also why we alias CTask=TapedTask when Libtask was re-designed. Hopefully, in the next few weeks, all these things will stabilise.

[devmotion]

I see, if there are more changes to the type to be expected in the next weeks it might be easier to update AdvancedPS once everything has stabilized. Maybe we could open an issue that reminds us about these abstract fields.

In general, I think we shouldn't update compat bounds before versions of upstream packages are released. I don't think it makes updates much faster and it means if accidentally something breaks users will actually end up with the broken versions. Whereas if we update compat bounds once upstream packages are released, hopefully our tests catch all (common) bugs.

[yebai]

I'm merging this PR, but we won't make a new release from master until #32 is complete. In addition, I believe some (minor) changes are required from the Turing side to become compatible with AdvancedPS#master.