-
Notifications
You must be signed in to change notification settings - Fork 21
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
added_multiprocessing #26
added_multiprocessing #26
Conversation
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.
- Remove header data added by spyder editor. We don't need those in this project.
- Change implementation to use Process class not thread pools. Thread pools are not interruptable i.e cannot be scheduled when in I/O wait.
- Do not bring change to code structure. You can create a sepparate utility class focused on Process based execution and then use that poool based execution to pass our function here along with args in kwargs fashion. This will allow us to isolate issues of Multiprocessing to one single file.
3c6c9fa
to
6829bb0
Compare
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.
- You are over complicating the implementation.
- I will add an interface to be implemented, follow that it will help you to implement the functionality
- have a habit of adding doc string to your code. I did not saw doc string for the new function and classes.
@abhishekgupta368 I will provide an interface make sure to implement that. |
@abhishekgupta368 discuss the implementation during meetup, please. It's not what we require. |
No, there is problem in my git terminal due to which you got problematic file. |
this is a error, I got:
|
Ya that's a legit error, because we are passing lambda functions which are not exactly pickable. i.e you cannot pickle them or covert to binary representation of objects. So hence the issue. Minor modifications should be able to help. You see if there is some hacky way of fixing this. 😅 I am looking into it too. |
ok |
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.
- No use of logger for task-related instantiation, the logger information is provided for usage , use accordingly.
- There is a merge conflict, resolve it ASAP
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.
- Can you put some log messages specific to this class. Which will inform us about current stage of the processes running.
- Log the stats
- add doc string to the classes and function. use vscode extension docstring for generating the format.
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.
Ok , looks good. merging.
hard work paid off |
#6 adding multiprocessing
Label: medium
Feature added:
I add multiprocessing in a file using pools