Skip to content

Conversation

@lasley
Copy link
Contributor

@lasley lasley commented Jan 9, 2018

This PR adds a multiprocessing pool for the repo aggregation if desired.

I am still testing this, but wanted to submit the PR early to make sure there's not somewhere else you'd rather I put this. The reasoning I'm thinking that might be the case is because it seems the method I modified is untested (at least directly), so I'm thinking it's not important.

  • Move repo aggregation into a helper method
  • Add option for the amount of processes to use when aggregating
  • Use multiprocessing pool for aggregation if aforementioned option is enabled

* Move repo aggregation into a helper method
* Add option for the amount of processes to use when aggregating
* Use multiprocessing pool for aggregation if aforementioned option is enabled
@crimoniv
Copy link
Contributor

crimoniv commented Feb 7, 2018

I also think it would be interesting to have this option, as maintaining a large amount of repositories highly increases the execution time (even though some of them are only a few KBs).

@sbidoul
Copy link
Member

sbidoul commented Feb 7, 2018

I'm personally fine with the principle of this PR. One must examine why travis is red though.

@crimoniv
Copy link
Contributor

I've submitted some changes to the main PR branch (LasLabs#1), feel free to comment or suggest any change.

Cheers!

@crimoniv
Copy link
Contributor

crimoniv commented Apr 6, 2018

Done! No more Travis errors 👌

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.953% when pulling 6890d06 on LasLabs:feature/thread-download into 90e77a2 on acsone:master.

1 similar comment
@coveralls
Copy link

coveralls commented Apr 9, 2018

Coverage Status

Coverage remained the same at 81.953% when pulling 6890d06 on LasLabs:feature/thread-download into 90e77a2 on acsone:master.

@crimoniv
Copy link
Contributor

We would love to see this feature being added to the main branch, would you mind if I open a new PR including our latest changes?

@sbidoul
Copy link
Member

sbidoul commented May 28, 2018

Hi, I had not noticed it's green now. I'm personally fine with this approach. Feel free to open another PR.

cc/ @lmignon

@sbidoul
Copy link
Member

sbidoul commented May 28, 2018

BTW, I just notice LasLabs#1 is much more complex than this one. What was not working with this PR?

@crimoniv
Copy link
Contributor

That PR includes tests and avoids the working directory lock -which prevented using a multithreading approach instead of a multiprocessing one-, among other minor fixes.

I will open a proper PR when I'm at home, thank you!

@sbidoul
Copy link
Member

sbidoul commented Aug 22, 2018

Superseded by #21

@sbidoul sbidoul closed this Aug 22, 2018
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.

4 participants