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

add ncpus_per_host option to LSFCluster #176

Closed
wants to merge 5 commits into from

Conversation

louisabraham
Copy link
Contributor

@louisabraham louisabraham commented Oct 17, 2018

fix #172

@louisabraham louisabraham changed the title solve #172 fix #172 Oct 17, 2018
@louisabraham louisabraham changed the title fix #172 add ncpus_per_host option to LSFCluster Oct 17, 2018
Copy link
Collaborator

@willirath willirath left a comment

Choose a reason for hiding this comment

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

There's a lot of re-formatted code that has nothing to do with the actual changes. This is a problem: git blame then won't be of any use in the affected lines, because it'll point to the commit that did the reformat rather than the one that implemented functionality.

I'd recommend reverting all but the changes that really change behaviour and, if necessary, deal with any code style issues separately.

@louisabraham
Copy link
Contributor Author

@willirath the reformatting is done in separate commits, I don't understand what you mean.

@willirath
Copy link
Collaborator

Sorry, I was not as verbose as I should have been: All these commits will be squashed to one commit representing this PR. Then, we'll have git blame telling the user that the reformatted lines have been changed in order to fix the LSFCluster issue.

@louisabraham
Copy link
Contributor Author

louisabraham commented Oct 17, 2018

I'm am no expert in github, but I think it only happens if you actually squash.

Since I forked directly from the current HEAD, you can actually do Rebase and merge.

A merge commit (the default) will also work.

If you prefer, I can cherry-pick the commits and turn off my editor's autoformatting.

@guillaumeeb
Copy link
Member

Hm, I think @willirath has got a point. It will be clearer to avoid reformatting for this issue.
The rule here is to only use squash and commit, so we will lose the detail commit history. If you could only implement the changes necessary to fix the identified issue this would be better.

@louisabraham
Copy link
Contributor Author

Ok, I'll redo it. Anyway I think I'll need to change my code depending on your answer on #172.

Just for my personal knowledge: what is the point of using only "squash and commit"?

@willirath
Copy link
Collaborator

Just for my personal knowledge: what is the point of using only "squash and commit"?

The master branch evolving in steps that make sense from a functional point of view rather than reflecting the (often random) steps it took to develop and implement the solution.

Taking this PR as an example, we'd have one commit telling the reader "LSFCluster now correctly starts all threads on the same node and here's all we did to achieve that." vs different commits that make it very hard to later understand what happened.

@guillaumeeb
Copy link
Member

Thanks anyway @louisabraham for all the work you've already done, and for sticking with this!
Sorry I was not really available today to answer your questions.

@louisabraham
Copy link
Contributor Author

I am the one who thanks both of you for actually taking more time to explain things to me than it would take you to fix the bug :)

@willirath
Copy link
Collaborator

I am the one who thanks both of you for actually taking more time to explain things to me than it would take you to fix the bug :)

I'm glad you take it this way! Communicating criticism in a well-meaning and constructive manner is not trivial and it'd probably be easy to read my remarks in an offensive tone.

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.

LSFCluster worker doesn't execute all threads on the same node
3 participants