Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tighten dask-core and distributed pinnings #190

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

charlesbluca
Copy link
Member

@charlesbluca charlesbluca commented Aug 9, 2022

As part of addressing dask/dask#9367, this PR tightens the pinning of the dask-core and distributed dependencies so that we can only pull in packages with an exact version match, preventing us from erroneously pulling in nightly packages.

cc @galipremsagar

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@charlesbluca
Copy link
Member Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/dask-feedstock/actions/runs/2826861231.

recipe/meta.yaml Outdated
Comment on lines 21 to 22
- dask-core {{ version }}
- distributed {{ version }}
- dask-core =={{ version }}
- distributed =={{ version }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So one downside of this particular pinning is that we are not able to pick up a hot-fix from dask-core or distributed (for example 2022.8.1.1) without a corresponding release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking about this - I wasn't sure how impactful this would be to the release process, considering that in the past, hotfixes have generally been done through a new patch release of all 3 packages. However, this obviously would limit our options moving forward.

Do you know of any way we can achieve the intended solution (excluding pre-alpha packages with the same version prefix) without using as tight of a pinning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL dask-core {{ version }} and dask-core =={{ version }} are different -- thanks @charlesbluca!

Re: hot-fixes, I agree with @charlesbluca that I don't think this will be an issue in practice given our current release process. We've already just issued a new release of all three packages which would, I think, work with the existing changes here. The only other thing that comes to mind is security backports (Xref dask/community#244) which we've yet to ever act on. IIUC this could throw a wrench into the current proposal in that community issue, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toying around with conda install pinnings, it looks like we actually can do excludes by regex without creating a tight pinning; for example, doing something like:

conda install "dask=2022.7.1,!=2022.7.1a.*"

Would end up picking up all 2022.7.1.* packages excluding those that match 2022.7.1a.*. I assume that we won't be suffixing packages with a.* unless they're pre-releases (feel free to point out any cases where we might need this), so happy to try out this option here if it unblocks the security backport proposal 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that we won't be suffixing packages with a.* unless they're pre-releases

Yeah, that's my understanding too

@jakirkham
Copy link
Member

cc @jrbourbeau (in case you have thoughts here :)

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. Had one suggestion below. Thanks Charles! 🙏

recipe/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <[email protected]>
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Charles! 🙏

@jakirkham
Copy link
Member

@jrbourbeau, could you please take another look? 🙂

@jakirkham
Copy link
Member

Planning on merging EOD if no comments

@jrbourbeau
Copy link
Member

Thanks @jakirkham for the ping -- I'm currently saturated with a few other things. Will plan to take a look at this in a bit, but also feel free to merge if that doesn't happen

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrbourbeau jrbourbeau merged commit 9c2d84e into conda-forge:main Aug 12, 2022
@jakirkham
Copy link
Member

Thanks James! 🙏

@charlesbluca, it's worth noting that we will need a repodata hotfix to apply this to older package versions as well

@charlesbluca
Copy link
Member Author

charlesbluca commented Aug 15, 2022

Might want to hold off on the repodata hotfix - I tried generating dask-core packages with the proposed security backport version scheme (2022.8.0.1 in this case) to see if they would work with the new conda-forge builds and it looks like that raises conflicts because we're pinning tightly to 2022.8.0:

$ mamba create -n test --override-channels -c ./main/ -c conda-forge dask==2022.8.0=*_1 dask-core==2022.8.0.1 --dry-run

Encountered problems while solving:
  - package dask-2022.8.0-pyhd8ed1ab_1 requires dask-core 2022.8.0,!=2022.8.0a*.*, but none of the providers can be installed

Need to play around with the conda recipe syntax a bit locally, but think we'll want something more like

    - dask-core {{ version }}.*,!={{ version }}a
    - distributed {{ version }}.*,!={{ version }}a

Not sure if we need to append a .* to these specifications or if that gets done manually; will try both out locally and open a new PR in each feedstock once I know the correct syntax.

EDIT:

Once that's done, we'll also want to make sure the repodata patch undoes the tight pinning we've set on these recent builds.

Comment on lines +21 to +22
- dask-core ={{ version }},!={{ version }}a*
- distributed ={{ version }},!={{ version }}a*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may need more tinkering, but this is one idea

Suggested change
- dask-core ={{ version }},!={{ version }}a*
- distributed ={{ version }},!={{ version }}a*
- dask-core =={{ version }}|{{ version }}.*,!={{ version }}a*
- distributed =={{ version }}|{{ version }}.*,!={{ version }}a*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants