Skip to content

Conversation

FredLoney
Copy link
Contributor

Make a SHA-1 digest directory name for long parameterizations.

@satra
Copy link
Member

satra commented Mar 21, 2013

this looks good - adding a couple of items for thought/implementation

  • a test
  • a config option (so that this doesn't happen by default - or does happen and people can turn it of
  • make sure datasink doesn't really on the filepath
  • why not reduce it to a single sha1 based on the entire path? this would render the working directory unreadable, but that would be ok in these circumstances.

@FredLoney
Copy link
Contributor Author

Thanks, Satra. I will make the change a config option and add a test.

I doubt if datasink would rely on the path, since it is a manufactured name. I will test this.

Reducing the entire path loses the hierarchical directory structure. I am hesitant to do so, since:

  1. I don't know if there are dependencies on this structure, or how to convincingly test this.
  2. There is value in having a directory name that is not completely opaque.

@FredLoney FredLoney closed this Mar 21, 2013
@FredLoney FredLoney reopened this Mar 21, 2013
@chrisgorgo
Copy link
Member

It seems that some things got mixed up in this pull request. It's probably because you have not used branches separating different features/bugfixes. We would still love to see these improvements in master, but please separate them into branches. Thanks!

@chrisgorgo chrisgorgo closed this May 1, 2013
@FredLoney
Copy link
Contributor Author

Hi Chris,

I thought you'd pull it into a branch. I will rebase and branch the change.

Fred

From: Chris Filo Gorgolewski <notifications@github.commailto:notifications@github.com>
Reply-To: nipy/nipype <reply@reply.github.commailto:reply@reply.github.com>
Date: Wednesday, May 1, 2013 11:28 AM
To: nipy/nipype <nipype@noreply.github.commailto:nipype@noreply.github.com>
Cc: Me <loneyf@ohsu.edumailto:loneyf@ohsu.edu>
Subject: Re: [nipype] SHA-1 parameterization working directory name (#535)

It seems that some things got mixed up in this pull request. It's probably because you have not used branches separating different features/bugfixes. We would still love to see these improvements in master, but please separate them into branches. Thanks!


Reply to this email directly or view it on GitHubhttps://github.com//pull/535#issuecomment-17297542.

@chrisgorgo chrisgorgo reopened this Jun 2, 2013
@chrisgorgo
Copy link
Member

Hi,
Thanks for rebasing, but it seems that some ANTS and dcmstack related commits are still mixed up. Could you look into it and separate them into different PRs? It seems that the current thing is a bit of a mess. What would be the simplest way to clean it up (@satra correct me if I'm wrong) is to make sure your master branch is up to date, create a brand new branch and cherry pick only the commits related to hash directies into it. Then you can send a new PR on that branch.

@FredLoney
Copy link
Contributor Author

Hi Chris,

My understanding is that the pull request was withdrawn pending isolation into a branch and adding a unit test. The FredLoney/nipype fork contains six branches merged into master which enable nipype to work in our environment. PR #535 is fixed in the sha-digest-dirname branch, which is rebased as of a week ago. The fork master should not be merged into nipy/nipype. I plan to submit a pull request specifying only the PR branch. Other branches implement different enhancements for which a PR has not yet been filed. These might result in separate pull requests.

Fred

From: Chris Filo Gorgolewski <notifications@github.commailto:notifications@github.com>
Reply-To: nipy/nipype <reply@reply.github.commailto:reply@reply.github.com>
Date: Saturday, June 1, 2013 5:19 PM
To: nipy/nipype <nipype@noreply.github.commailto:nipype@noreply.github.com>
Cc: Me <loneyf@ohsu.edumailto:loneyf@ohsu.edu>
Subject: Re: [nipype] SHA-1 parameterization working directory name (#535)

Hi,
Thanks for rebasing, but it seems that some ANTS and dcmstack related commits are still mixed up. Could you look into it and separate them into different PRs? It seems that the current thing is a bit of a mess. What would be the simplest way to clean it up (@satrahttps://github.com/satra correct me if I'm wrong) is to make sure your master branch is up to date, create a brand new branch and cherry pick only the commits related to hash directies into it. Then you can send a new PR on that branch.


Reply to this email directly or view it on GitHubhttps://github.com//pull/535#issuecomment-18799515.

@chrisgorgo
Copy link
Member

Ok thanks - looking forward to your PRs!

@chrisgorgo chrisgorgo closed this Jun 4, 2013
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.

3 participants