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

Update tcia prostatex model notebook #1852

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kirbyju
Copy link
Contributor

@kirbyju kirbyju commented Oct 5, 2024

Description

Fixed dependency issues with itkwidgets

Fixed some dependency issues related to itkwidgets that were preventing the notebook from running.
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 9, 2024

Looks the notebook still can not pass the premerge.
https://github.com/Project-MONAI/tutorials/actions/runs/11241293887/job/31252532094?pr=1852

DCO Remediation Commit for Justin Kirby <[email protected]>

I, Justin Kirby <[email protected]>, hereby add my Signed-off-by to this commit: 00f3362
I, Justin Kirby <[email protected]>, hereby add my Signed-off-by to this commit: 6e36c64
I, Justin Kirby <[email protected]>, hereby add my Signed-off-by to this commit: f3d172e
I, Justin Kirby <[email protected]>, hereby add my Signed-off-by to this commit: 6fc1435

Signed-off-by: Justin Kirby <[email protected]>
@kirbyju
Copy link
Contributor Author

kirbyju commented Oct 9, 2024

Looks the notebook still can not pass the premerge. https://github.com/Project-MONAI/tutorials/actions/runs/11241293887/job/31252532094?pr=1852

I'm not sure what the issue is. I can run the notebook end to end in both Colab and Sagemaker Studio Lab without any problems other than these reported dependency issues that don't seem to affect the rest of the notebook:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
jupyter-events 0.5.0 requires jsonschema[format-nongpl]>=4.3.0, but you have jsonschema 3.2.0 which is incompatible.
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
pydicom-seg 0.4.0 requires jsonschema<4.0.0,>=3.2.0, but you have jsonschema 4.23.0 which is incompatible.

@thewtex
Copy link
Collaborator

thewtex commented Oct 9, 2024

Just an observation, it looks like CI is timing out (the limit is 6 hours):

image

@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 9, 2024

Just an observation, it looks like CI is timing out (the limit is 6 hours):

Yes, but the main reason here is that it hangs at specific cell since the whole notebook don't take over 6 hours to run.

Fix pep8/premerge errors.

Signed-off-by: Justin Kirby <[email protected]>
@kirbyju
Copy link
Contributor Author

kirbyju commented Oct 9, 2024

Just an observation, it looks like CI is timing out (the limit is 6 hours):

Yes, but the main reason here is that it hangs at specific cell since the whole notebook don't take over 6 hours to run.

I just saw this in the middle of the report:

Running ./model_zoo/TCIA_PROSTATEx_Prostate_MRI_Anatomy_Model.ipynb
Checking PEP8 compliance...
stdin:78:1: F401 'sys' imported but unused
import sys
^
stdin:102:1: E303 too many blank lines (3)
# + [markdown] id="oYxvN-rebvwG"  # noqa: E501
^
Error: Try running with autofixes: --autofix.

Check failed!

I don't understand why it's complaining about sys not being used. I guess it doesn't pick it up properly when calling it like this for all the installations?

!{sys.executable}

I changed it back to just say "python" instead and removed the extra blank lines.

@kirbyju
Copy link
Contributor Author

kirbyju commented Oct 10, 2024

I'm out of ideas in terms of fixing whatever is still causing this last check to fail.

@KumoLiu
Copy link
Contributor

KumoLiu commented Oct 10, 2024

Can you try fix the version to 1.0a23? From what I tested last time, I find this version will not hang. But the latest version will hang.

Removing screenshots of viewers to see if that fixes pre-merge checks.

Signed-off-by: Justin Kirby <[email protected]>
@kirbyju
Copy link
Contributor Author

kirbyju commented Oct 10, 2024

That was already part of the requested changes. It's trying to use 1.0a53 in the most recent commits:

!python -m pip install --upgrade -q \"itkwidgets[all]==1.0a53\" imjoy_elfinder

I tried removing the image ouputs from the imjoy plugin to see if maybe that was the issue.

@ericspod
Copy link
Member

I attempted to run this notebook on a headless machine that we have for CICD. I found that the view cell just before "6. MONAI Model Inference" runs quickly but produces no view window, only messages about removing duplicate codecs. No other cell after running this will ever complete, so something changes when this is run to break the interpreter. This is with itkwidgets[all]==1.0a53.

I also found that requests needs to be explicitly installed when creating a fresh conda environment.

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