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

Fix perl 5.18+ by adding sorting to hash keys where necessary #912

Merged
merged 6 commits into from Jan 25, 2018

Conversation

Projects
None yet
5 participants
@cmdcolin
Contributor

cmdcolin commented Jul 27, 2017

This is a fix for #470

It addresses hash order randomization issues by just adding sorting to keys when they are evaluated.

This fully improves the excessive running time and huge disk space issues encountered on perl 5.18+

This also tests for travis-ci to 5.16 and 5.18 (default for travis is to test 5.14). We could add more perl versions if there is interest but testing 5.16 and 5.18 gives us the "before & after hash order randomization test suite".

@cmdcolin cmdcolin removed the in progress label Jul 27, 2017

@rbuels rbuels self-assigned this Jan 25, 2018

@rbuels rbuels added this to the 1.12.4 milestone Jan 25, 2018

@rbuels

This comment has been minimized.

Collaborator

rbuels commented Jan 25, 2018

@cmdcolin Why does sorting hash keys fix this issue? What has changed from earlier perl versions that is breaking things?

@keiranmraine

This comment has been minimized.

Contributor

keiranmraine commented Jan 25, 2018

We saw this in our tools too, it appears that perl 5.18+ actually enforces a more random response when sort is not specified (tends to be 2 states). I think the idea being to highlight that although it has historically been stable on a platform/OS it isn't guaranteed between systems.

Also worth checking that there isn't any keys $hashref as that is now:

Starting with Perl 5.14, an experimental feature allowed keys to take a scalar expression. This experiment has been deemed unsuccessful, and was removed as of Perl 5.24.

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Jan 25, 2018

Looks like this feature that landed in 5.18 http://www.perlmonks.org/?node_id=1005122

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Jan 25, 2018

Also as to why sorting fixes it, there are just certain tests that expect certain data to be in a certain order in the JSON or similar.

@rbuels rbuels merged commit f49444d into master Jan 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@enjoyyourcopepods

This comment has been minimized.

enjoyyourcopepods commented Jan 27, 2018

I am wondering if the strange JSON files that were produced by the random hash ordering might cause problems with the way JBrowse displays annotations tracks.

@cmdcolin cmdcolin deleted the fix_perl_518 branch Feb 6, 2018

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Feb 16, 2018

I don't believe this would have caused any problems with how tracks are displayed. It just caused many permutation of typed arrays to be created. You might open another issue if you have seen errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment