Skip to content

Support cross 32bit/64bit pickles for decision tree #21552

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

Merged
merged 24 commits into from
Nov 25, 2021

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Nov 4, 2021

Reference Issues/PRs

Fix #19602.

What does this implement/fix? Explain your changes.

This makes it possible to save a decision tree pickle on a 64bit machine and load it on a 32bit machine (as well as the other way around although this is probably not that useful in practice).

The test uses a similar approach as #21539: it checks that a pickle containing arrays of a different bitness (e.g. 64bit arrays on a 32bit machine) can be loaded fine. This is not exactly what we want to test, i.e. generating a pickle on 64bit machine and load it on a 32 bit machine but is close enough.

If we want, we could have an additional test that is exactly the use case we are aiming to support. We can generate pickle bytes on a 64 bit machine (we can put a comment indicating how to generate it if we need to regenerate it) and load it on the 32 bit CI.

Any other comments?

#21539 should be merged first, putting this one in draft for now.

Testing a similar thing for more/other estimators is left for a future PR.

@lesteve lesteve force-pushed the cross-32bit-64bit-tree-pickle branch from 66d4660 to 808662b Compare November 4, 2021 14:42
@lesteve lesteve marked this pull request as ready for review November 8, 2021 16:14
@jjerphan jjerphan self-requested a review November 16, 2021 14:51
@glemaitre glemaitre self-requested a review November 16, 2021 14:56
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Thanks @lesteve, here is a first pass:

lesteve and others added 5 commits November 18, 2021 15:23
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

A first shallow review on this IMO support.

Current tests LGTM. 👍
I think that I still need to understand the details and associated challenges of cross 32 and 64 bit support to fully help with this PR.

lesteve and others added 5 commits November 19, 2021 14:57
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
wip
Copy link
Member

@ogrisel ogrisel 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 very much @lesteve for this epic PR :)

My comment are mostly nitpicky naming suggestions that you should feel be free to ignore (but I find my names more descriptive ;) and a couple more simple comments.

@ogrisel
Copy link
Member

ogrisel commented Nov 24, 2021

Maybe @rth is interested in giving this PR a final review?

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough work! LGTM as well.

@rth rth merged commit 8dcde1e into scikit-learn:main Nov 25, 2021
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre pushed a commit that referenced this pull request Dec 25, 2021
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@lesteve lesteve deleted the cross-32bit-64bit-tree-pickle branch March 31, 2023 09:40
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.

Tree pickle portability between 64bit and 32bit arch
4 participants