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

Noninteractive run of notebooks and flake8 checks #107

Merged
merged 63 commits into from
Jan 27, 2021

Conversation

rijobro
Copy link
Contributor

@rijobro rijobro commented Jan 13, 2021

This uses the python package papermill to run notebooks. papermill is used instead of nbconvert --execute as it displays a progress bar. Without some sort of progress update, it's impossible to tell if execution is slow or is hanging.

Use sed to change some variables to 1 to reduce time (e.g., max_epochs = 180 -> max_epochs = 1).

Use jupytext to internally convert from notebook (*.ipynb) into script (*.py). From here, flake8 is used for checking PEP8 compliance, and autopep8, isort, black and autoflake are used for autofixes.

This PR also contains all the changes to make the notebooks PEP8 compliant.

The most important file to review here is the runner.sh script. All other files are hopefully only PEP8 changes, but I would be grateful for any checks to make sure I haven't done anything silly.

Example usage:

# run for 1 file with autofix
runner.sh --autofix --file modules/load_medical_images.ipynb

# run on all files, skip notebook execution
runner.sh --no-run

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rijobro
Copy link
Contributor Author

rijobro commented Jan 13, 2021

fixes #102
fixes #101
fixes #6
also only uses pip install if the package is missing. Fixes #50

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for the huge effort in this PR.
Overall looks good to me, I didn't check every single line as it's really too big...
And put some comments inline.

Thanks.

@@ -138,8 +125,8 @@
"source": [
"## Setup data directory\n",
"\n",
"You can specify a directory with the `MONAI_DATA_DIRECTORY` environment variable. \n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these blanks in Markdown doc are to make the next sentence in a new line, that's Markdown gramma I think.
Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be tricky to solve, but thanks for pointing it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed a temporary fix f7deb35 pending answer from here: mwouts/jupytext#723.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, could you please update the PR to ignore the blanks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done that, but I'll have to revert all these accidental changes manually.

acceleration/transform_speed.ipynb Outdated Show resolved Hide resolved
modules/nifti_read_example.ipynb Outdated Show resolved Hide resolved
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jan 26, 2021

Hi @rijobro ,

Is this PR OK for review again?

Thanks.

@rijobro
Copy link
Contributor Author

rijobro commented Jan 27, 2021

hi @Nic-Ma , yes it is now!

Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @rijobro for looking into these, this is a major enhancement

@wyli wyli requested a review from Nic-Ma January 27, 2021 14:45
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your quick update.
Looks good to me.

@wyli wyli merged commit 59b57a4 into Project-MONAI:master Jan 27, 2021
@rijobro rijobro deleted the noninteractive_run branch January 27, 2021 18:38
boneseva pushed a commit to boneseva/MONAI-tutorials that referenced this pull request Apr 21, 2024
* start of automatic and update occ. sens.

* current progress

* current progress

* current progress

* current progress

* vae

* models ensemble

* layer_wise_learning_rate

* gan

* 3d_classification

* add 3d_segmentation as well as flake

* transform speed

* autopep8

* update runner

* flake8 support added

* max_num_epochs->max_epochs

* check that max_epochs exists unless not expected

* uncomment executing notebook

* check pip install

* ignore temp files

* no noqa for indented import monai

* dont check for indented import monai

* flake8 changes

* magic pip installs

* autofixes

* use ! instead of % for pip install

* so far

* current progress. add black, isort, autoflake

* current progress

* update class lung lesion notebook

* class lung lesion

* current progress

* current progress

* finished

* finished

* remove pip install of pinned pytorch version

* = list() to = []

* [DLMED] fix dyunet notebook issue

Signed-off-by: Nic Ma <nma@nvidia.com>

* correct faulty import

* pep8 for dynunet

* all working

* add missing quotations

* remove personal file path

* remove NiftiDataset

* dont remove EOL whitespace from comments

* 2d_classification

* re-add spaces

* spaces

* last spaces

* final changes

* notification at end

* notification on exit

* add github action

* data subfolder

* make folder if necessary

* add flake8 to requirements

* change test name

Co-authored-by: Nic Ma <nma@nvidia.com>
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.

3 participants