MedicalImageAnalysisTutorials / SlicerCochlea

Multi-modal cochlea Images registration, segmentation and analysis
13 stars 4 forks source link

ImportError: no module named request #15

Closed jamesobutler closed 5 years ago

jamesobutler commented 5 years ago

There is an import error that is causing failing tests for the extension with Slicer 4.10.

This is the result of the change from

from six.moves.urllib.request import urlopen, urlretrieve

to

import urllib.request

This occurred in 044b8206cf0a53dffd5cc5c524ced556cef17d45.

You can have code written that is both python2 and python3 compatible which is what one my previous PR attempted to do. This will reduce maintenance for both Slicer 4.10 and Slicer Preview.

You can also have two branches where one is for python2 and the other for python3. It appears you have started to do this with your 4.10.1 branch. However, you haven’t updated the SlicerExtensionsIndex 4.10 branch to use this new python2 compatible branch. It is still specifying to use the master branch. See here

idhamari commented 5 years ago

thanks, I didn't know that! i just updated SlicerExtensionsIndex and add 2 new pull requests to solve the problem.

jamesobutler commented 5 years ago

Just a note, you probably want to change your branch name from “4.10.1” to something like “4.10”. This would clarify it will work for all Slicer 4.10.X versions and not just Slicer 4.10.1. For instance, Slicer 4.10.2 was released (see here)

idhamari commented 5 years ago

update: I re-forked the ExtensionsIndex and it seems there is no conflict in the updated two pull requests. Please merge them so I can close the opened issues here.

jamesobutler commented 5 years ago

I approved the PRs but I don’t have write access to ExtensionsIndex so someone else will have to merge.

Please keep these issues opened until the extensions are built again for Slicer 4.10 stable and Slicer Preview. Then we can check if the specific failing tests have been solved. I plan to check at 09:00 EST tomorrow assuming the PRs are merged today.

jamesobutler commented 5 years ago

The specific issue related to “request” is no longer present in the lastest test. There are other unrelated failing test issues now.