- 
                Notifications
    You must be signed in to change notification settings 
- Fork 80
Notebook CI Job -- report times and fix regression #1746
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
Conversation
| Results from serial execution on the CI: 24 mins total  | 
| After 8370d88, 16 mins total and  | 
4dc1f6e    to
    59597bb      
    Compare
  
    | with 8 workers, total time of the execution step is 6m17s | 
| without maxtasksperchild=1, the total time of the execution step is 6m22s and doesn't show any of the stdout until the very end. I'll keep that change in | 
| only_out_of_date: Only re-execute and re-export notebooks whose output files | ||
| are out of date. | ||
| n_workers: If set to 1, do not use parallelization. Otherwise, use | ||
| `multiprocessing.Pool(n_workers)` to execute notebooks in parallel. | 
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.
The way the argument parser is currently defined, if the user does not supply a value for --n-workers, the value will be None, which multiprocessing.Pool(…) will interpret as "use as many as there are CPU cores on the machine". Although it's true that the docstring above is technically correct, the reader would have to know about the behavior of multiprocessing.Pool(None) to deduce how many workers will be used. IMHO a sentence here would be helpful to explain the connection between not providing a value and the resulting number of processes used.
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 still suggest a note in the docstring to mention the behavior, but it's up to you.
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.
yep i'll add that
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.
LGTM, with a suggestion about adding a sentence to the docstring.
Uh oh!
There was an error while loading. Please reload this page.