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

threadsafe job counts; configurable batch_size #581

Merged
merged 3 commits into from
Jan 15, 2016
Merged

Conversation

gojomo
Copy link
Collaborator

@gojomo gojomo commented Jan 12, 2016

Fix for #571. Uses local vars in each thread for counts. Adds class/init batch_words parameter to set a non-default value.

@gojomo
Copy link
Collaborator Author

gojomo commented Jan 15, 2016

@tmylk @piskvorky – any concerns about merging this into develop? I believe it will improve (and perhaps fix entirely) two causes of intermittent CI test failures (and thus save anyone else working on develop from thinking their changes may have caused a failure, as in #538).

@piskvorky
Copy link
Owner

It looks quite elegant, I'm not sure why I messed around with shared state when a simple counter was enough.

Maybe I'm missing something here (reasoning about parallelism is tricky) but if all tests and sanity checks pass, it's LGTM from me (+ the obligatory CHANGELOG entry).

@piskvorky
Copy link
Owner

@tmylk I suspect a similar issue could be behind the failing tests in LdaMulticore (which uses processes instead of threads, but otherwise similar logic flow, AFAIR).

gojomo added a commit that referenced this pull request Jan 15, 2016
threadsafe job counts; configurable batch_size
@gojomo gojomo merged commit f267abf into develop Jan 15, 2016
@gojomo
Copy link
Collaborator Author

gojomo commented Jan 15, 2016

Added CHANGELOG & merged to make it easier for other in-progress/future branches to build on this.

@piskvorky piskvorky deleted the threadsafe_job_sizing branch January 16, 2016 09:25
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.

2 participants