Release 2.29.0 #98
Comments
|
We might want to address dask/dask#6694 before
releasing. I don't know if it's a blocker, but reverting the commit that
caused the regression should be relatively pain free.
…On Thu, Oct 1, 2020 at 2:58 PM James Bourbeau ***@***.***> wrote:
If there aren't objections, I'd like to push Dask and Distributed 2.29.0
out tomorrow afternoon as part of our current weekly release schedule
cc @jakirkham <https://github.com/jakirkham> @kkraus14
<https://github.com/kkraus14>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#98>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKAOIQDN37BUCZKSK3LMSDSITNMPANCNFSM4SA32R4A>
.
|
|
It would be nice to get PR ( dask/distributed#4138 ) in. Would get rid of some warnings that have been cropping up in CI. Should be a quick review if anyone has a moment 😉 |
It sounds like we should revert dask/dask#6510 and issue a release with it reverted to solve the performance issue. I'll follow up on the weekly release meta issue though as this is revealing a problem with weekly releases when we try to make non-trivial changes to Dask. |
|
FYI: dask/dask#6699 should fix the performance issue with dask/dask#6510 |
|
I'm taking a look at this now.
…On Fri, Oct 2, 2020 at 5:32 AM Mads R. B. Kristensen < ***@***.***> wrote:
FYI: dask/dask#6699 <dask/dask#6699> should fix
the performance issue with dask/dask#6510
<dask/dask#6510>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#98 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTG4FNF5YF5ZDW5FKZTSIXB6PANCNFSM4SA32R4A>
.
|
|
After looking at this briefly I think that @madsbk fix will be fine, however I think it makes more sense logistically to revert in this case, release, let things settle over the weekend, and then go on a merging spree (there is a lot of interesting work in both dask/dask and dask/distributed). @jrbourbeau are you still planning to handle the release today? If not I'm happy to do it. |
|
I'm still planning to handle the release today. From reading through the discussion here and in other related issues, it seems like others are on board for @mrocklin's proposal to fix the recent performance regression by merging dask/dask#6697, then release and let things sit over the weekend. Then next week we revert dask/dask#6697 and merge a few other PRs. I'll hold off a few hours on merging dask/dask#6697 and releasing to allow time for feedback, but that's my current plan. I'm lacking a little bit on context when it comes to @madsbk's PR dask/dask#6699. Is this a temporary workaround to undo the performance regression (an alternative to dask/dask#6697), or is this a change we want to make and keep longer term? |
|
Mads' workaround is a proposed permanent solution. Nick, who alerted us to the performance regression has tested out that fix and it works fine. If we wanted to move forward with it I would be fine, but I'm personally inclined to hold off. |
|
As Nick has already confirmed that fixes the issue, I would suggest (though acknowledge I'm biased) going ahead with the fix. That should allow these new HLG changes to get into more users hands, which in turn should help us further refine them based on feedback that we would otherwise not get by reverting. That said, I don't have strong feelings here 🙂 |
|
If we're doing this much merge revert etc... on a Friday after 5 PM, maybe we shouldn't have a release. This feels like a good time to not have a release weekly and just give time for all these changes to get tested and aligned a little more. #84 (comment) I feel like this is a good time to delay a release :) |
In general I support this spirit of getting things out there. In this particular case though the HLG work won't be of much benefit to people without some of the follow-on PRs. Pragmatically I don't think it matters either way, but given that one person had an issues I'm inclined to just play it safe on this one.
There are important bug fixes and performance improvements in master that are not in the latest release. I see very mild risk in merging in Mads fix, and near-zero risk in reverting. The risk of not releasing seems higher to me (we know that we're giving wrong results in dask.bag.groupby today). |
|
I think #84 (comment) is proposing we do release today, after merging dask/dask#6697 which undoes some changes that lead to a performance regression. Then merge more PRs and wait a bit before releasing again. This would let us, as you've suggested, take some time to ensure things are better tested and a little more aligned. |
|
Yeah, I'd definitely like to get what we have in master out now so that we're not holding up important bugfixes while we let larger changes marinate . |
|
Agreed it would be good to get our current set of bugfixes out. Based on #98 (comment), #98 (comment), and #98 (comment) I'm going to say that a majority of us are generally okay with merging dask/dask#6697 and releasing. To @datametrician's point about giving some extra time test changes out, let's plan to not release next week so we can merge in PRs with significant changes and have (at least) two weeks to thoroughly test them out. |
|
Closing as 2.29.0 is on PyPI, conda-forge, and Docker hub. Thanks all for the conversation here, I'm looking forward to testing out changes over the next couple of weeks |
If there aren't objections, I'd like to push Dask and Distributed 2.29.0 out tomorrow afternoon as part of our current weekly release schedule
cc @jakirkham @kkraus14
The text was updated successfully, but these errors were encountered: