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

[CIVIS-1508] DEP upgrade joblib dep #429

Merged
merged 6 commits into from
Nov 5, 2021

Conversation

dleatherman-civis
Copy link
Contributor

@dleatherman-civis dleatherman-civis commented Nov 5, 2021

Features

This PR upgrades the joblib dependency to allow versions 1.1.x which provides several enhancements and bug fixes around pickling which impacted previous runs.

Closes #423.

dleatherman-civis and others added 4 commits October 14, 2021 10:31
joblib is now past 1.1.x and we should be able to install later version.
This is so a newer minor version cannot be downloaded on accident.
@dleatherman-civis dleatherman-civis changed the title Feature/upgrade joblib dep [ISSUE #423] upgrade joblib dep Nov 5, 2021
Copy link
Member

@jacksonlee-civis jacksonlee-civis left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I have a high-level question:

  • Assuming joblib follows semantic versioning, the jump from joblib version 0.* to 1.* could mean breaking changes from joblib. If there are any, would these changes affect civis-python in any way?

Also, it looks like you might have started this work before some of the most recent PRs got merged into master. Some of the no-longer-valid changes show up in your PR here and should be dropped.

I've taken the liberty to update this PR's title to link this work to the relevant JIRA ticket, and added Closes #423 to the PR description so that GitHub will close the issue once this PR is merged.

CHANGELOG.md Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@jacksonlee-civis jacksonlee-civis changed the title [ISSUE #423] upgrade joblib dep [CIVIS-1508] DEP upgrade joblib dep Nov 5, 2021
This is so this dependency needs to be updated less.
@dleatherman-civis
Copy link
Contributor Author

Thank you for the contribution! I have a high-level question:

  • Assuming joblib follows semantic versioning, the jump from joblib version 0.* to 1.* could mean breaking changes from joblib. If there are any, would these changes affect civis-python in any way?

Also, it looks like you might have started this work before some of the most recent PRs got merged into master. Some of the no-longer-valid changes show up in your PR here and should be dropped.

I've taken the liberty to update this PR's title to link this work to the relevant JIRA ticket, and added Closes #423 to the PR description so that GitHub will close the issue once this PR is merged.

The breaking change is identified here. with joblib <1.x, pickling joblib.Memory objects was backwards compatible. Newer versions of numpy changed how numpy.dtypes was pickled which caused objects that should be the same to create different hashes. Essentially, if you're loading in joblib.Memory pickle objects, they are no longer compatible across versions (hence the major version bump).

Copy link
Member

@jacksonlee-civis jacksonlee-civis left a comment

Choose a reason for hiding this comment

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

Thank you for tracking down joblib's breaking changes. Looks like joblib 1.* (and numpy >= 1.20) have been out for about a year, which shouldn't cause issues for us.

LGTM!

@dleatherman-civis dleatherman-civis merged commit a83bd47 into master Nov 5, 2021
@jacksonlee-civis jacksonlee-civis deleted the feature/upgrade-joblib-dep branch November 9, 2021 18:32
@jacksonlee-civis jacksonlee-civis added this to the v1.16.0 milestone Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Joblib 1.x
2 participants