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

fix: change how hash values are computed #1174

Merged
merged 6 commits into from Sep 11, 2015
Merged

Conversation

satra
Copy link
Member

@satra satra commented Aug 10, 2015

@chrisfilo, @hjmjohnson, @oesteban - please review this PR.

while working on the py3 branch, i realized that the way we were calculating the hash values left it up to a random number generator whenever a dict was used. this fixes that. what this does mean is that every current working directory will be rendered invalid.

@hjmjohnson
Copy link
Contributor

@satra Is there a way to use a configuration variable to control which of the two methods is used?
1) Defaults to current method until the next major nipy release is made, then changes to the new method. In 1 year, the current method will be planned to be removed completely.
2) Does this affect every pipeline, or only pipelines that use dict?

Thanks,
Hans

@satra
Copy link
Member Author

satra commented Aug 10, 2015

@hjmjohnson - this is technically and actually a bug (unless i missed something in my crazy two days of looking over 800+ files). so please do verify that i'm doing the correct thing.

being a bug, i would rather not have a config option. essentially, every input spec is converted to a dict and is hashed. the problem was that we were storing the hashed values back in a dict and then hashing the whole thing by converting the dict to a string representation. python doesn't guarantee order of elements in a dict.

strictly speaking this is one thing we should even backport to older versions.

that being said, you can always run a workflow with updatehash=True if you are confident that no inputs have changed.

@hjmjohnson
Copy link
Contributor

@satra : I forgot about "updatehash=True " That addresses all my concerns, and provides a way to continue using an existing internal cache while updating nipype.

I think that "updatehash=True" provides all the functionality needed to prevent years of data re-processing.

Hans

@hjmjohnson
Copy link
Contributor

@satra : This looks good to me. If I understand correctly, you convert the dict to a list where the elements of the list are sorted versions of the dictionary keys.

This approach should give consistent orderings in all cases.

* upstream/master: (76 commits)
  Revert "attempt to add coverage"
  attempt to add coverage
  run tests in source folder, remove test isolation
  Don't run TBSS in parallel.
  less
  force better memory management
  fewer subjects
  updated doctest
  updated tests
  make bedpostx test faster
  match default parameters to those from FSL
  Various fixed to BEDPOSTX interface - previous version was not operational
  make eddy correction tests quicker by trimming the volume
  BUG: Registration interface failed multi-modal
  lower the requirements of the bedpostx test
  DTI related fixes.
  diffusion workflow tests fixes
  make it more verbose
  configure location of the test data with env
  fixed nosetest call
  ...
* upstream/master:
  STYLE: Autogenerated files ignored
  ENH: More end-of-line whitespace removed.
  ENH: Improve the removal of trailing whitespace
  updted changelog
  remove tests
  added info about composite transforms
  BUG: Restore registration interface failed multi-modal
  STYLE: Remove end of line trailing white space
  fixed doctests
  fixed syntax
  there is always only one composite transform - a list is not appropriate here
  Current ANTS doctests are only valid for unimodal registrations
  Revert "BUG: Registration interface failed multi-modal"
  better handling of errors when using caching
  out dir for epi_reg is compulsory but meaningless for nipype
  added circleCI and codacy badges
  fixed dir making
  adding tests
  adding further options to 3drefit
* upstream/master:
  fix: deploy only nipy/nipype master branch
  fix for identifying if one or multiple durations are employed to describe the condition
  corrected line of modelgen and added test for variable concatenated events
  fix
  test
  fix: bad copy paste.
  tst: added more duration checking tests
  tst: added duration checking
  fix: use float for computation input
  fix: update scripts to use single file ants composite_transform output
  doc: changed description
  fix: address multimodal and unimodal inputs
  Update test_auto_MRIConvert.py
  FIX: add TR, TE and TI flag to FreeSurfer's MRIConvert
chrisgorgo added a commit that referenced this pull request Sep 11, 2015
fix: change how hash values are computed
@chrisgorgo chrisgorgo merged commit f882fd2 into nipy:master Sep 11, 2015
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.

None yet

3 participants