search

dask / community

For general discussion and community planning. Discussion issues welcome.
20 stars 3 forks source link

Release 2022.05.1 #245

Closed jsignell closed 2 years ago

jsignell commented 2 years ago

Apologies for opening this rather late in the day. Normally we would release 2022.05.1 tomorrow, but I heard from @jrbourbeau (who heard it from @crusaderky I think) that there is a release blocker on distributed. I am proposing that we delay the release until Friday 2022-05-19.

cc @jakirkham @quasiben @jsignell @fjetter @crusaderky

crusaderky commented 2 years ago

This is the blocker, ready for review and merge:

The above PR doesn't fully fix the issue https://github.com/dask/distributed/issues/6305, but it undoes the regression from 2022.05.0.

jakirkham commented 2 years ago

We also ran into this issue recently:

cc @pentschev

crusaderky commented 2 years ago

Correction - https://github.com/dask/distributed/pull/6217, which happened after 2022.05.0, may have introduced a regression in the resumed state. Discussion on https://github.com/dask/distributed/issues/6305.

jrbourbeau commented 2 years ago

Checking in here for releasing tomorrow. It looks like @fjetter has a PR open (xref https://github.com/dask/distributed/pull/6363) that will close https://github.com/dask/distributed/issues/6320. Are there other PRs in distributed we want to make sure are merged before releasing? (cc @fjetter @crusaderky)

crusaderky commented 2 years ago

@jrbourbeau https://github.com/dask/distributed/issues/6305 is a regression introduced after the last release, and quite a nasty one IMHO. Not sure about @fjetter's timeline for it.

jrbourbeau commented 2 years ago

Okay, thanks for flagging @crusaderky. I'll check in tomorrow as it's well past @fjetter normal working time. @fjetter if you're able to provide any additional context around https://github.com/dask/distributed/issues/6305 that would be appreciated

fjetter commented 2 years ago
jrbourbeau commented 2 years ago

Thanks for the update and all your work on those issues @fjetter. @quasiben @pentschev @jakirkham could you provide some feedback on if https://github.com/dask/distributed/pull/6363 is good to go?

jrbourbeau commented 2 years ago

@gjoseph92 reviewed https://github.com/dask/distributed/pull/6363 and has some concerns https://github.com/dask/distributed/pull/6363#pullrequestreview-980540137. Given these concerns, that it's nearing EOD for US-based folks, and European-based folks are already done for the day, I'm going to hold off on releasing until someone like @fjetter @crusaderky can take a look at @gjoseph92's feedback. We don't have to wait another full week to release, but I would like to give some time for https://github.com/dask/distributed/pull/6363#pullrequestreview-980540137 to be looked at

gjoseph92 commented 2 years ago

There is also https://github.com/dask/distributed/pull/6270#issuecomment-1133358073, which I think would be a blocker on its own as well.

jakirkham commented 2 years ago

It would be nice to see some new issues to track these things. Discussions and requests in closed issues/PRs are hard to track

graingert commented 2 years ago

a PR for just removing that stray breakpoint: https://github.com/dask/distributed/pull/6417

fjetter commented 2 years ago

A few notes about the process

  1. I 100% agree with @jakirkham that we should open new tickets about new release blockers. As soon as something is in main, we require a new ticket. From what I understand reading through the comments, we have two blockers for which we have one PR comment and one ticket

Is there anything else?

  1. I think for announcement tickets like "Release XY" we should keep the top-level comment up-to-date such that new readers can come to the issue and see what the current status is without going through the entire ticket itself. Currently, to understand where we are, one needs to read the entire ticket and carefully read every comment to find out what our current decision is. it's also a bit difficult to distinguish what belongs to the previous release attempt vs the current release attempt vs the next release attempt.

I opened https://github.com/dask/community/issues/247 to discuss

  1. Most importantly, I think all owners and core maintainers should put an emphasize on fixing and/or reviewing release blockers asap. It took a very long time until a fix was actually proposed and with https://github.com/dask/distributed/pull/6363 it was difficult and slow to get feedback about whether this would fix the issue. Nobody actually reviewed the PR until very late on Friday, after the PR was merged. I felt comfortable with the proposed changes (https://github.com/dask/distributed/pull/6363#issuecomment-1133157875) and while likely not everybody will be able to spot content problems (like https://github.com/dask/distributed/pull/6363#pullrequestreview-980540137), everybody should be able to perform a sanity check to spot things like https://github.com/dask/distributed/pull/6417. That includes me as well, I should not even push code like this but mistakes happen and this is what a review is for. We should not merge PRs without any review.
mrocklin commented 2 years ago

If we revert https://github.com/dask/distributed/pull/6363 and https://github.com/dask/distributed/pull/6270 then can we safely release?

gjoseph92 commented 2 years ago

@mrocklin yes, but I think https://github.com/dask/distributed/pull/6363 was considered a fix for https://github.com/dask/distributed/issues/6320, which was considered a blocker?

jrbourbeau commented 2 years ago

I think we can safely revert https://github.com/dask/distributed/pull/6270 or merge https://github.com/dask/distributed/pull/6408 which aims to address the security concerns (cc @jacobtomlinson for confirmation). https://github.com/dask/distributed/pull/6363 on the other hand was a fix for this issue https://github.com/dask/distributed/issues/6320 which, I believe, would break Dask-CUDA if we released without a fix. I'd like someone like @fjetter to weight in on @gjoseph92's review comments here https://github.com/dask/distributed/pull/6363#pullrequestreview-980540137 (at least the review comments that he marked with a ⚠️) to see if they constitute blocking the release and, if so, what changes we want to make to resolve the concerns that were raised

gjoseph92 commented 2 years ago

It would be great to get confirmation from @pentschev @jakirkham @quasiben that https://github.com/dask/distributed/issues/6320 is indeed a release blocker. Otherwise temporarily reverting Florian's PR is definitely the fastest path forward.

jacobtomlinson commented 2 years ago

For the HTTP API I would prefer to go with the alternative fix that @gjoseph92 suggested in https://github.com/dask/distributed/pull/6408#issuecomment-1134800571.

quasiben commented 2 years ago

Either way with https://github.com/dask/distributed/issues/6320 there will be an issue: a known bug in dask-cuda or recent concerns raised @gjoseph92 . I would say should hold off on the release until @gjoseph92 has resolved his concerns and if we can help, we can prioritize the investigation.

mrocklin commented 2 years ago

I would say should hold off on the release until @gjoseph92 has resolved his concerns and if we can help, we can prioritize the investigation.

If you all can jump in here that would also be welcome. I think that @gjoseph92 is trying to address a couple of things blocking the release today.

gjoseph92 commented 2 years ago

@quasiben and I talked:

jacobtomlinson commented 2 years ago

I've raised https://github.com/dask/distributed/pull/6420. I'm on a train with poor cell reception so please dont hesitate to just push review feedback directly to my PR.

jrbourbeau commented 2 years ago

Brief update:

Once https://github.com/dask/distributed/pull/6423 is in we'll be in good shape to push out the release (xref https://github.com/dask/distributed/pull/6423#issuecomment-1135283456)

jrbourbeau commented 2 years ago

https://github.com/dask/distributed/pull/6423 is in (thanks to all who engaged!) so I'm going to start pushing out the release

jrbourbeau commented 2 years ago

2022.05.1 is out on PyPI and conda-forge PRs are open

jrbourbeau commented 2 years ago

Thanks all!

mrocklin commented 1 year ago

So it sounds like "no", simply reverting would not solve the problem. Is that correct?

On Mon, May 23, 2022 at 9:45 AM Gabe Joseph @.***> wrote:

@mrocklin https://github.com/mrocklin yes, but I think dask/distributed#6363 https://github.com/dask/distributed/pull/6363 was considered a fix for dask/distributed#6320 https://github.com/dask/distributed/issues/6320, which was considered a blocker?

— Reply to this email directly, view it on GitHub https://github.com/dask/community/issues/245#issuecomment-1134769684, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKZTBNEOT2ONSD265VBG3VLOKZDANCNFSM5VZLSVXA . You are receiving this because you were mentioned.Message ID: @.***>