-
Notifications
You must be signed in to change notification settings - Fork 29
clic #116
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
base: main
Are you sure you want to change the base?
clic #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I like the structure of this, but I'm not sure about lazy_import
.
It also looks like the conda environment is unused... in the other modules, the workers run in separate processes in the conda environment. Doing that would generally be more portable, but I think it may be incompatible with the threading model you have here.
Since you check dependencies and prompt before changing the environment, I'd say this could be merged as is, but I encourage you to look into using SlicerConda like the other modules instead of the main process for better portability.
Hi @allemangD, I’ve implemented the requested changes in CLIC. Thanks for your feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did not explain my prior comment about the conda environment well. Apologies for any confusion. See details in the new inline comment.
self._toggle_ui(True); t0=time.time() | ||
if not self._ensure_env(): | ||
self._toggle_ui(False); return | ||
self._flush_q() | ||
|
||
# import logic once env is ready | ||
if not self.logic: | ||
from CLI.CLICLogic import CLICLogic | ||
self.logic = CLICLogic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not prepare the environment as you expect. You should be able to self.conda.condaCreateEnv
to prepare a conda environment with torch and the other dependencies - it won't modify the slicer environment, where the UI and _on_predict
run. You'd need to move the processing code into a different script, and use self.conda.condaRunFilePython
to run that script in the conda environment.
The threading model that you had before seemed fairly robust - it may be easier to simply revert these conda changes and use that instead, but I worry about cross-platform support and support on future versions of Slicer.
So I defer to you on which route to take:
- either switch back to the threading model you had before, removing the conda usage entirely
- or try to move the computation into a script that runs in the Conda environment that you prepared in the latest changes.
For reference, the "threading model" I refer to is this, from the prior revision of this PR.
SlicerAutomatedDentalTools/CLIC/CLIC.py
Lines 261 to 310 in 1efceef
def onPredictButton(self): | |
if not self.check_dependencies(): | |
return | |
if not self.input_path: | |
qt.QMessageBox.warning(self.parent, 'Warning', 'Please select an input file/folder') | |
return | |
if not self.model_folder: | |
qt.QMessageBox.warning(self.parent, 'Warning', 'Please select a model folder') | |
return | |
param = { | |
"input_path": self.input_path, | |
"model_folder": self.model_folder, | |
"output_dir": self.output_folder, | |
"suffix": self.ui.suffixLineEdit.text | |
} | |
def update_progress(progress): | |
self.ui_queue.put(("progress", progress)) | |
def update_log(message): | |
self.ui_queue.put(("log", message)) | |
def display_callback(action, file_path): | |
self.ui_queue.put((action, file_path)) | |
self.segmentationLoadedEvent = threading.Event() | |
try: | |
self.RunningUI(True) | |
self.processThread = threading.Thread( | |
target=self.logic.process, | |
args=(param, update_progress, update_log, display_callback, self.segmentationLoadedEvent) | |
) | |
self.processThread.start() | |
import time | |
start_time = time.time() | |
while self.processThread.is_alive(): | |
slicer.app.processEvents() | |
current_time = time.time() | |
self.ui.TimerLabel.setText(f"Time elapsed: {current_time - start_time:.2f}s") | |
self.process_ui_queue() | |
time.sleep(0.1) | |
self.process_ui_queue() | |
self.ui_queue.put(("log", "Segmentation completed successfully!")) | |
except Exception as e: | |
self.ui_queue.put(("log", f"An error occurred during segmentation: {str(e)}")) | |
finally: | |
self.RunningUI(False) |
If you choose to move things to the Conda environment, you could use the ALI module as a reference. These sections in particular:
https://github.com/DCBIA-OrthoLab/SlicerAutomatedDentalTools/blob/main/ALI/ALI.py#L1107-L1191
https://github.com/DCBIA-OrthoLab/SlicerAutomatedDentalTools/blob/main/ALI/ALI.py#L1817-L1851
hello @allemangD I think that now here is only my commit, sorry