Skip to content

Multiprocessing support to pyjscompressor.py #764

Closed
wants to merge 3 commits into from

2 participants

@happyalu

This adds a -j option to pyjscompress (like make), to speed up compression by using python's multiprocessing.

@happyalu happyalu closed this Jul 29, 2012
@happyalu happyalu reopened this Jul 29, 2012
@anthonyrisinger
Polyglot Pythonistas member

on the whole your changes look good/an improvement, but you've made far too many style changes for me to be able to review this without reviewing the file in entirety.

style changes must to be isolated in their own commit, free of any functional changes. if you can create a pull request with only the bare minimum needed to implement multiprocessing i will be happy to review and merge!

once that's merged, you can certainly submit a pull request to add option parsing, and/or better style, in two additional pull requests. in general, each request should encapsulate a single functional change else the review time grows exponentially.

lastly, pyjs needs to support python 2.5+, so things like argparse won't be available -- optparse must be used instead.

thanks @happyalu! just create another request with the multiprocessing bits and we'll go from there.

@happyalu

@xtfxme Commit 6375156 only has style changes, and commit 5248c57 only has the stuff required for multiprocessing. I had to change some things (such as, using tempfile module instead of temp/ directory as a part of the multiprocessing stuff.

About the style changes: they are mostly related to lines longer than 80 chars.

I am not sure how to create pull requests of single commits :) Github seems to have pushed the entire branch out as pull request. What do you recommend?

@happyalu

Also, argparse falls back to optparse in 5248c57.. and I tested the fallback to work.. but I could only check it on Python 2.7

@anthonyrisinger
Polyglot Pythonistas member

@happyalu, ah ok, i see that now ... i didn't notice among all the green and red ;-)

a fallback in the manner you've written would be fine. as long as it added in an isolated commit/pull-request i've no problem pulling it.

@anthonyrisinger
Polyglot Pythonistas member

@happyalu, oops i missed you first response.

github links a pull request to a branch in your repository, so each pull request needs to be a separate branch; this also lets you work on issues independently. for projects i contribute to, i usually make a branch like defect/description-of-the-issue or feature/something-new-i-added then open a request from that branch.

don't get me wrong, from what i see your changes are all improvements, i just need them incrementally else i can't review well ... the file is currently 170 LOC but your pull req shows a doubling of that, essentially a rewrite. again not a problem in the end, but needs to be pulled in a controlled manner.

simply make a branch, commit the absolute bare minimum needed to support multiprocessing, then push to github + open a request. you can later start another branch adding option handling, and finally a third with style changes. i'd love to pull everything you have, but i'm tight on resources and need to be able to review quickly.

@happyalu

@xtfxme no I totally understand :) I created a separate branch for the style changes. It would be easier for me if the style changes (pep8 related) are merged first.. Since the multiprocessing code was written after that .. :-P

I hope that is ok!

Do you recommend I bundle option handling with multiprocessing changes? (since -j is the option for supporting multiproc) Or should I divide those changes into two?

@anthonyrisinger
Polyglot Pythonistas member

@happyalu, i would prefer the option changes to be separate from multiprocessing as well. you can add multiprocessing first and simply default to using it -- i see no reason why multiprocessing can't be used by default :-)

once thats's added, you can definitely clean up/add option handling, and thus add the ability to force a certain -j <num>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.