-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/concurrent downloads #110
Conversation
Task is an abstract base class that implements a method for executing sub tasks by the process pool.
Travis is failing because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach.
When I run it as-is with emoji.list -p /tmp/
then I get a lot of these exceptions:
Traceback (most recent call last):
File "/usr/local/Cellar/python/3.6.5_1/Frameworks/Python.framework/Versions/3.6/lib/python3.6/multiprocessing/queues.py", line 234, in _feed
obj = _ForkingPickler.dumps(obj)
File "/usr/local/Cellar/python/3.6.5_1/Frameworks/Python.framework/Versions/3.6/lib/python3.6/multiprocessing/reduction.py", line 51, in dumps
cls(buf, protocol).dump(obj)
TypeError: can't pickle _thread.RLock objects
self.emoji_name = emoji_name | ||
self.url = url | ||
self.save_path = save_path | ||
self.logger = logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For private fields, it's customary to prefix with __
.
Regarding the log output, it might be best to buffer that until it either succeeds or fails, and then post to the actual logger? Then it won't mess with the stdout until ready/finished. |
69b782e
to
b995099
Compare
Interesting that you're getting those exceptions. It seems to be related to Python version
The REPL is now blocked until the tasks have completed. Blocking should be fine in this case since a user shouldn't be able to execute multiple commands while others are running, that could get messy IMO. Although it could be interesting to support optional multiple command execution as a command line multi-flag, such as
|
b995099
to
fbfddc8
Compare
I was able to repro the issue with Python 3.6 running in a Docker container.
Instead of passing the logger directly to the tasks, the logs are being buffered and the caller can choose to replay the logs or not. This seems to fix the issue where things weren't able to be locked, can you give it a try now? |
Now it works great! Thanks! |
Wanted to get some initial feedback on this.
Changes proposed in this pull request:
This isn't quite ready to be merged since the save file after the downloads have finished isn't being generated. There is now weirdness with the logger, since things are running concurrently the main REPL will return immediately after calling
execute_tasks
, and logs will start showing up afterwards moving theSlacker >
prompt out of context. Will need to decide how to best reset the prompt.