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

Runilastik module that can use local OR Docker installations #226

Merged
merged 21 commits into from
Apr 7, 2024

Conversation

bethac07
Copy link
Member

We were hoping the Predict module would actually be able to work with Windows in analysis mode, but alas it seems still very hard - it seems to not be a matter of how CellProfiler CALLS ilastik, but how ilastik is interacting with the threading model. I still haven't given up hope that it's fixable, I think it might be that all we need to do is to rather than try to keep it in the same thread, use Popen to give it its own - but for now, that's causing weirdness with the tempfiles. It might be not only solvable but fast to solve, just haven't taken the time to dig in.

In the meantime, Suganya was working on this Dockerized version. I can confirm it works in local and Docker mode on Mac (and it does contain all the improvements in #225). On a Windows VM, I can't test the Dockerized version, because you can't install Docker Desktop on a Windows VM (apparently AWS blocks doing virtualization turtles all the way down). On the VM, though, the local version works in test mode, and (as expected) not in analysis mode, on both CellProfiler running from source and the latest 4.2.6 build. Suganya though is getting weird errors on her PC, which could be her-PC specific or not. Will need to get hands-on to hers or another physical Windows machine to check, but wanted to mark the place we were at.

@bethac07
Copy link
Member Author

Now working - turns out not to have been a Windows-vs-Mac thing but a using-a-grayscale-vs-a-color-test-image thing. Who knew!

Needs tests and documentation pushes, @sugan89 , and I guess we should consider at this point move predict to unmaintained, but yay, working!

@bethac07
Copy link
Member Author

One more note to add - we were originally thinking of letting the user provide their own ilastik Docker, if they don't like ours, but realized that in this container the version name is in the executable path, so in order to support multiple versions of our own we'd need to add a dictionary of Dockers to abspaths, and add a setting if the user chooses "other" to let them provide the abspath to the executable. Not impossible, but likely low-priority for now; adding this comment so we remember in the future why it's set up this way and our plan for expanding if/when needed.

The Docker that is used to run this module can be found here - https://hub.docker.com/layers/biocontainers/ilastik/1.4.0_cv2/images/sha256-0ccbca62d9efc63918d9de3b9b2bb5b1265a084f8b6410fd8c34e62869549791?context=explore
"""

#Link to the ilastik biocontainer. We should make changes in the module such that the user will be able to choose any ilastik docker they would like.
Copy link
Member Author

Choose a reason for hiding this comment

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

Can remove

if self.docker_choice == "select your own":
vis_settings += [self.custom_docker_name, self.docker_executable]

if self.docker_or_local.value == "Local":
Copy link
Member Author

Choose a reason for hiding this comment

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

else

"Analysis mode will take a long time to run using Docker",
self.docker_or_local,
)
if self.docker_or_local.value == "Local":
Copy link
Member Author

Choose a reason for hiding this comment

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

else

"-v", f"{model_directory}:/model",
f"{ILASTIK_DOCKER_choice}", f"{ILASTIK_command}", "--headless",
"--project", f"/model/{os.path.basename(model_file)}"
] # '"/opt/ilastik-1.4.0-Linux/run_ilastik.sh"' this command is specific to the ilastik biocontainer
Copy link
Member Author

Choose a reason for hiding this comment

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

Can remove comment

active_plugins/runilastik.py Outdated Show resolved Hide resolved
@bethac07
Copy link
Member Author

bethac07 commented Feb 6, 2024

https://plugins.cellprofiler.org/using_plugins.html#using-docker-to-bypass-installation-requirements should explicitly say to go look at the table (yes, it says above, but if your'e just linked to that section, you might miss it); above needs to be changed to no longer say it only supports RunCellpose!

@bethac07 bethac07 marked this pull request as ready for review April 7, 2024 13:45
@bethac07 bethac07 merged commit 11b46ee into master Apr 7, 2024
3 of 6 checks passed
@bethac07 bethac07 deleted the Runilastik_test branch April 7, 2024 13:46
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.

2 participants