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

Browser not working in Sllicer 5.6.2 or 5.6.1 #36

Closed
pieper opened this issue May 7, 2024 · 21 comments · Fixed by ImagingDataCommons/idc-index#77 or #38
Closed

Browser not working in Sllicer 5.6.2 or 5.6.1 #36

pieper opened this issue May 7, 2024 · 21 comments · Fixed by ImagingDataCommons/idc-index#77 or #38

Comments

@pieper
Copy link
Member

pieper commented May 7, 2024

Install on a freshly downloaded current stable release of Slicer (5.6.2) results in this error:

image

The problem is repeatable after restarting Slicer.

With 5.6.1 I get this error:

...
uccessfully installed duckdb-0.10.2 idc-index-0.5.4 idc-index-data-18.0.1 pandas-2.1.4 pyarrow-16.0.0 pytz-2024.1 s5cmd-0.1.0 tqdm-4.66.4 tzdata-2024.1
Traceback (most recent call last):
  File "/Applications/Slicer-5.6.1.app/Contents/Extensions-32438/IDCBrowser/lib/Slicer-5.6/qt-scripted-modules/IDCBrowser.py", line 73, in setup
    self.logic.setupPythonRequirements()
  File "/Applications/Slicer-5.6.1.app/Contents/Extensions-32438/IDCBrowser/lib/Slicer-5.6/qt-scripted-modules/IDCBrowser.py", line 1427, in setupPythonRequirements
    self.idc_version = index.idc_version
AttributeError: module 'idc_index.index' has no attribute 'idc_version
@fedorov
Copy link
Member

fedorov commented May 9, 2024

@pieper I asked @vkt1414 to look into this, and he traced it down to idc-index and the seemingly harmless one-line change in ImagingDataCommons/idc-index#76. We suspect that change that preserved NULL values led to unintended reset to NULL of the concatenation result for the distinct values of patient-level attributes. Vamsi is working on a fix for this and on adding tests to cover this situation in idc-index.

@vkt1414
Copy link
Collaborator

vkt1414 commented May 9, 2024

@pieper I asked @vkt1414 to look into this, and he traced it down to idc-index and the seemingly harmless one-line change in ImagingDataCommons/idc-index#76. We suspect that change that preserved NULL values led to unintended reset to NULL of the concatenation result for the distinct values of patient-level attributes. Vamsi is working on a fix for this and on adding tests to cover this situation in idc-index.

Submitted a PR already ImagingDataCommons/idc-index#77

@fedorov
Copy link
Member

fedorov commented May 9, 2024

Reopening, since we need to confirm this is fixed in the Slicer binary, and we need to investigate the second error in 5.6.1 which may or may not be related to ImagingDataCommons/idc-index#69.

@fedorov fedorov reopened this May 9, 2024
@vkt1414
Copy link
Collaborator

vkt1414 commented May 9, 2024

Reopening, since we need to confirm this is fixed in the Slicer binary

Theoretically, this should be independent of the Slicer binary. However, the new download hierarchy enabled by default is going to bring another issue. (I injected the index.py file from the main branch into slicer python env libs to test)
I'll submit another PR to disable hierarchy for now in SlicerIDCBrowser

image

@vkt1414
Copy link
Collaborator

vkt1414 commented May 9, 2024

even after merging ImagingDataCommons/idc-index#78

I would not be able to fix the download issue even after adding dirTemplate=None as I can not understand what images.zip is here.

https://github.com/ImagingDataCommons/SlicerIDCBrowser/blob/main/IDCBrowser/IDCBrowser.py#L1061-L1070

if I change downloadDir=self.extractedFilesDirectory to downloadDir=Path(downloadFolderPath) path is resolved properly and files are downloaded but cannot be loaded into the scene.

I'm also seeing this on both windows and ubuntu
image

@vkt1414
Copy link
Collaborator

vkt1414 commented May 9, 2024

@pieper the easiest to be able to get around temporarily is fix the idc-index version to 0.5.2

So please uninstall idc-index and idc-index-data with slicer.util.pip_uninstall and install idc-index==0.5.2

@pieper
Copy link
Member Author

pieper commented May 10, 2024

@vkt1414 thanks for the work on this and the workaround info. I don't have the VM where this occurred anymore, but it'll be great to have this fixed for future installations.

@vkt1414
Copy link
Collaborator

vkt1414 commented May 10, 2024

@vkt1414 thanks for the work on this and the workaround info. I don't have the VM where this occurred anymore, but it'll be great to have this fixed for future installations.

Thanks for reporting the error!

Yes. The fix in idc-index took care of this issue and we are working on fixing the download now.

In my opinion this experience underscores the need to fix idc-index version to ensure consistent experience across users. We must also either upgrade or degrade any existing idc-index or idc-index-data versions.

Secondly I really wish we could either test SlicerIDCbrowser in the idc-index CI pipeline or set up a payload hook to trigger tests in SlicerIDCbrowser.

@Steve, Do you know of any extensions that use GitHub actions for CI without waiting to check on cdash dashboard?

@pieper
Copy link
Member Author

pieper commented May 10, 2024

@Steve, Do you know of any extensions that use GitHub actions for CI without waiting to check on cdash dashboard?

No, I think we have focused on cdash for consistency. It would be helpful to try to fit into that framework but if there are convincing reasons to add some extract testing methods that would also be fine.

@fedorov
Copy link
Member

fedorov commented May 10, 2024

I would not be able to fix the download issue even after adding dirTemplate=None as I can not understand what images.zip is here.

If you look at the code, that fileName is not used. The extension was working. I do agree it has a lot of garbage code and needs to be completely rewritten.

@fedorov
Copy link
Member

fedorov commented May 10, 2024

if I change downloadDir=self.extractedFilesDirectory to downloadDir=Path(downloadFolderPath) path is resolved properly and files are downloaded but cannot be loaded into the scene.

Did you debug why it fails? Maybe it is just failing when you try to download for the first time because the top-level directory is missing?

@vkt1414
Copy link
Collaborator

vkt1414 commented May 10, 2024

if I change downloadDir=self.extractedFilesDirectory to downloadDir=Path(downloadFolderPath) path is resolved properly and files are downloaded but cannot be loaded into the scene.

Did you debug why it fails? Maybe it is just failing when you try to download for the first time because the top-level directory is missing?

The last known working idc-index version is 0.5.2 and there we don't check if downloadDir exists. s5cmd must be creating on the fly as I thought..
https://github.com/ImagingDataCommons/idc-index/blob/0.5.2/idc_index/index.py#L317

@pieper
Copy link
Member Author

pieper commented May 14, 2024

On 5.6.2 I'm still being offered the IDC extension from April, even though the build is working https://slicer.cdash.org/builds/3397595

Tried both mac and linux

@vkt1414
Copy link
Collaborator

vkt1414 commented May 14, 2024

I think that's accurate, @pieper. The last commit on the repo is on April 23. If the IDCBrowser is not functional, please uninstall idc-index and idc-index-data. When IDCBrowser is initialized again, it will install the latest index and index data. All builds should be functional at least on Linux as far I have tested.

image

@pieper
Copy link
Member Author

pieper commented May 14, 2024

I did a fresh install of 5.6.2 on linux and no matter what I try to access I get this:

image

@pieper pieper closed this as completed May 14, 2024
@vkt1414 vkt1414 reopened this May 14, 2024
@vkt1414
Copy link
Collaborator

vkt1414 commented May 14, 2024

Sorry @pieper, I forgot about that. I addressed the downloadDir error in #37

@fedorov I propose for the interim, we immediately fix idc-index to 0.5.4 and merge #37 and then figure out how to incorporate hierarchy in a later pull request.

@fedorov
Copy link
Member

fedorov commented May 14, 2024

@vkt1414 I do not see how it would help. Based on what I can tell, the check for the presence of the destination directory is happening before the call to s5cmd here: https://github.com/ImagingDataCommons/idc-index/blame/main/idc_index/index.py#L1143 (and it looks like was introduced in the progress reporting commit here ImagingDataCommons/idc-index@213b5c7).

I am going to submit a PR trying to fix this.

What I cannot understand is why it works with the preview release. It should not!

@vkt1414
Copy link
Collaborator

vkt1414 commented May 14, 2024

I did not understand what you meant.

Previously download_dicom_series used to have its own logic. Now all download from uuids must go through download from selection. And the first thing we do is to check whether the download directory actually exists. https://github.com/ImagingDataCommons/idc-index/blob/main/idc_index/index.py#L1143

In slicerIDCBrowser that is images folder. In main branch, images folder is never created. The PR #37 creates that directory.

@vkt1414
Copy link
Collaborator

vkt1414 commented May 14, 2024

What I cannot understand is why it works with the preview release. It should not!

As steve reported, it does not work on any new slicer version, except for those who have installed the extension from the time, before idc-index version 0.5.4 was released.

@fedorov
Copy link
Member

fedorov commented May 15, 2024

I tested 5.6.2 today, and download appears to work!

@pieper please reopen if you continue to see issues.

@pieper
Copy link
Member Author

pieper commented May 15, 2024

Yes, it's working today 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants