Release 2022.05.1

[jsignell]

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]

This is the blocker, ready for review and merge:

• Validate and debug state machine on handle_compute_task distributed#6327

The above PR doesn't fully fix the issue dask/distributed#6305, but it undoes the regression from 2022.05.0.

[jakirkham]

We also ran into this issue recently:

• dask-worker process remains alive after Nanny exception on plugins= distributed#6320

cc @pentschev

[crusaderky]

Correction - dask/distributed#6217, which happened after 2022.05.0, may have introduced a regression in the resumed state. Discussion on dask/distributed#6305.

[jrbourbeau]

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

[crusaderky]

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

[jrbourbeau]

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 dask/distributed#6305 that would be appreciated

[fjetter]

• Remove report and safe from Worker.close distributed#6363 should be ready to merge to address dask-worker process remains alive after Nanny exception on plugins= distributed#6320 If nobody raises any further concern, I assume this fixes the upstream issues
• We are struggling a bit to close test_stress_scatter_death distributed#6305 However, I'm increasingly confident that this should not be flagged as a release blocker, see also Fix test_stress_scatter_death distributed#6404 for more info

[jrbourbeau]

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

[jrbourbeau]

@gjoseph92 reviewed dask/distributed#6363 and has some concerns dask/distributed#6363 (review). 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 dask/distributed#6363 (review) to be looked at

[gjoseph92]

There is also dask/distributed#6270 (comment), which I think would be a blocker on its own as well.

[jakirkham]

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]

a PR for just removing that stray breakpoint: dask/distributed#6417

[fjetter]

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

• Don't expose insecure HTTP API distributed#6407
• Remove report and safe from Worker.close distributed#6363 (review)

Is there anything else?

2. 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 #247 to discuss

3. 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 Remove report and safe from Worker.close distributed#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 (Remove report and safe from Worker.close distributed#6363 (comment)) and while likely not everybody will be able to spot content problems (like Remove report and safe from Worker.close distributed#6363 (review)), everybody should be able to perform a sanity check to spot things like remove stray breakpoint distributed#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]

If we revert dask/distributed#6363 and dask/distributed#6270 then can we safely release?

[gjoseph92]

@mrocklin yes, but I think dask/distributed#6363 was considered a fix for dask/distributed#6320, which was considered a blocker?

[jrbourbeau]

I think we can safely revert dask/distributed#6270 or merge dask/distributed#6408 which aims to address the security concerns (cc @jacobtomlinson for confirmation). dask/distributed#6363 on the other hand was a fix for this issue dask/distributed#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 dask/distributed#6363 (review) (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]

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

[jacobtomlinson]

For the HTTP API I would prefer to go with the alternative fix that @gjoseph92 suggested in dask/distributed#6408 (comment).

[quasiben]

Either way with dask/distributed#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]

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]

@quasiben and I talked:

• @jacobtomlinson will handle Don't expose insecure HTTP API distributed#6407 with the approach in Don't expose HTTP API for secure clusters distributed#6408 (comment)
• I will make a follow-up PR fixing Add HTTP API to scheduler distributed#6270
  • Switch Worker.close back to closing the nanny by default, instead of restarting it
  • Remove half-closed state in Scheduler.handle_worker_status_change Remove report and safe from Worker.close distributed#6363 (comment)
  • I may also remove the Worker.terminate vs Worker.close; this might make more sense to handle as part of Restart worker via Nanny on connection failure distributed#6387. TBD.
• @quasiben will confirm my changes still resolve dask-worker process remains alive after Nanny exception on plugins= distributed#6320

[jacobtomlinson]

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

[jrbourbeau]

Brief update:

• Disable HTTP API by default distributed#6420 has been merged (thanks @gjoseph92 @jacobtomlinson @fjetter) which should workaround Don't expose insecure HTTP API distributed#6407 for us to release
• @gjoseph92 is working to close the remaining issue Regressions and potential bugs from #6363 distributed#6422 over in Follow-up on removing report and safe from Worker.close distributed#6423

Once dask/distributed#6423 is in we'll be in good shape to push out the release (xref dask/distributed#6423 (comment))

[jrbourbeau]

dask/distributed#6423 is in (thanks to all who engaged!) so I'm going to start pushing out the release

[jrbourbeau]

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

[jrbourbeau]

Thanks all!

[mrocklin]

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 <dask/distributed#6363> was considered a fix for dask/distributed#6320 <dask/distributed#6320>, which was considered a blocker? — Reply to this email directly, view it on GitHub <#245 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKZTBNEOT2ONSD265VBG3VLOKZDANCNFSM5VZLSVXA> . You are receiving this because you were mentioned.Message ID: ***@***.***>