Skip to content

feat: add IVIM workflow DAG with MinIO integration and platform docum…#112

Merged
oliverchampion merged 14 commits into
OSIPI:mainfrom
Unique-Usman:usman/kaapana_ivim_osipi
Sep 10, 2025
Merged

feat: add IVIM workflow DAG with MinIO integration and platform docum…#112
oliverchampion merged 14 commits into
OSIPI:mainfrom
Unique-Usman:usman/kaapana_ivim_osipi

Conversation

@Unique-Usman
Copy link
Copy Markdown
Contributor

…entation

  • Created a custom Airflow DAG for Kaapana that:

    • Retrieves input files (e.g., bvec, bval) from MinIO using MinioOperator
    • Processes data via IVIM Operator using nifti_wrapper_kaapana.py
    • Saves outputs back to MinIO using MinioOperator
    • Utilizes OPERATOR_IN_DIR and OPERATOR_OUT_DIR volume mounts
    • Cleans up intermediate files with CleanOperator
  • Added diagram (workflow_extension.png) showing DAG structure

  • Structured project under kaapana_ivim_osipi/ with clear separation of Docker, workflows, and processing code

  • Documented Otsus method extension installation steps

  • README: Added full Kaapana platform installation guide

…entation

- Created a custom Airflow DAG for Kaapana that:
  - Retrieves input files (e.g., bvec, bval) from MinIO using MinioOperator
  - Processes data via IVIM Operator using nifti_wrapper_kaapana.py
  - Saves outputs back to MinIO using MinioOperator
  - Utilizes OPERATOR_IN_DIR and OPERATOR_OUT_DIR volume mounts
  - Cleans up intermediate files with CleanOperator

- Added diagram (workflow_extension.png) showing DAG structure
- Structured project under `kaapana_ivim_osipi/` with clear separation of Docker, workflows, and processing code
- Documented Otsus method extension installation steps
- README: Added full Kaapana platform installation guide
Copy link
Copy Markdown
Contributor

@etpeterson etpeterson left a comment

Choose a reason for hiding this comment

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

first glance - more to come

Comment thread kaapana_ivim_osipi/Workflow_Extension/README.md Outdated
@@ -0,0 +1,8 @@
FROM local-only/base-installer:latest
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a special tag? How does this work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is. It is made available by the kaapana team

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One thing we can do here to avoid requiring multiple dockerfiles is define the base image as a build-time argument. We can set reasonable defaults so that the image builds and runs for regular users, and if a user wants to build for kaapana they can pass in build arguments for that (which would be described in the readme).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems very similar to the current Dockerfile: https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/blob/main/Docker/Dockerfile
Could they be unified in some way, perhaps with variables or sections?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should be able to unified it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This too seems like essentially a duplicate of something that already exists: https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/blob/main/WrapImage/nifti_wrapper.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we should be able to unified this also.

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

Hi @etpeterson, thanks for the review. Sorry for lot of noise in the commit. I did not meant for this current implementation to be perfect. I will keep making push here. It will serve as a synchronization for what the current state of the implementation is and also to get earlier review for whatever implementation is being done and also to ensure everyone is on the same page.

- Updated Chart.yaml with accurate description, tags, and maintainers for IVIM task force
- Removed unused UI form fields in DAG definition
- Fixed tuple syntax for file extensions
- make `nifti_wrapper_kaapana.py` to drop batch loop and use direct input/output paths
since our workflow is not batch.
@Unique-Usman Unique-Usman requested a review from etpeterson July 27, 2025 07:24
@ltorres6 ltorres6 self-requested a review July 28, 2025 14:49
Copy link
Copy Markdown

@ltorres6 ltorres6 left a comment

Choose a reason for hiding this comment

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

Leaving some questions, comments.

@@ -0,0 +1,8 @@
FROM local-only/base-installer:latest
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One thing we can do here to avoid requiring multiple dockerfiles is define the base image as a build-time argument. We can set reasonable defaults so that the image builds and runs for regular users, and if a user wants to build for kaapana they can pass in build arguments for that (which would be described in the readme).

Comment thread kaapana_ivim_osipi/README.md
Comment thread kaapana_ivim_osipi/Workflow_Extension/README.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you point me to where you got the original implementation of the fitting to compare?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

… dynamic UI configuration

This change makes the IVIM fitting pipeline fully configurable from the UI form instead of relying on hardcoded file paths and algorithm settings.

Key updates:
- UI form schema changes:
  - Added `source_files` for comma-separated NIfTI, bvec, bval paths (used for MinIO data retrieval)
  - Added optional `affine` matrix override
  - Added `algorithm` selection from a predefined list
  - Removed unused `single_execution` field and added `required` validation
- MyIvimFittingOperator:
  - Increased default execution timeout to 5 hours
  - Added configurable `ram_mem_mb` and `ram_mem_mb_lmt`
- nifti_wrapper_kaapana.py:
  - Read config from `$WORKFLOW_DIR/conf/conf.json` (no hardcoded paths)
  - Dynamically resolve input/output paths and algorithm from config
  - Support optional affine override
  - Added workaround to copy `.nii.gz` files to `WORKFLOW_DIR` to handle MinioOperator limitations

Motivation:
- Eliminate hardcoded values for input files and algorithm parameters
- Allow workflows to be driven entirely by user-provided configuration
- Improve maintainability and flexibility for different datasets and algorithms
Copy link
Copy Markdown
Contributor

@etpeterson etpeterson left a comment

Choose a reason for hiding this comment

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

I took a brief look. Let me know if you want something more thorough.

- name: dag-installer-chart
version: 0.0.0
repository: "file:///home/ivim/kaapana/services/utils/dag-installer-chart/"
repository: "file:///home/usman/kaapana/services/utils/dag-installer-chart/"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be hard coded for you only.

Copy link
Copy Markdown
Contributor Author

@Unique-Usman Unique-Usman Aug 29, 2025

Choose a reason for hiding this comment

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

we can make the file portable so it that we would not have to hardcoded. We basically bring the dag-installer-chart into the workflow, but, we might have to figure out what to do if the kaapana teams make an update to it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it could be relative or absolute. It just have to point to the dependency folder dag-installer-chart which is inside the kaapana repository/directory. I will make sure to add it to the docs.

# Temporary solution to pass through the kaapana MinioOperator limitation
# where it uses the same source_files for both put and get actions

# Find all .nii.gz files in the input directory
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need all files here or just the output files?

This update extends the IVIM fitting pipeline to handle both NIfTI and
raw DICOM uploads with a branching DAG structure.

Key changes:
- Added new DicomToNiftiOperator for converting DICOMs to NIfTI.
- Introduced new `dicom_to_nifti` processing container with Dockerfile,
  conversion script, and requirements.
- Updated DAG (dag_ivim-fitting_method.py):
  - Added `upload_type` field in UI form (`nifti` | `dicom`).
  - Implemented BranchPythonOperator to route workflow based on input type.
  - Defined parallel execution paths for DICOM and NIfTI.
  - Separated output MinIO upload tasks per path with cleanup.
- Updated nifti_wrapper_kaapana.py:
  - Added support for resolving inputs from DICOM-converted batches.
  - Preallocated arrays for IVIM fitting outputs (f, Dp, D).
  - Improved logging and error handling for missing batch files.
  - Added clear section headers and structure for maintainability.
- Clone TF2.4_IVIM-MRI_CodeCollection from GitHub in Dockerfile
- Moved nifti_wrapper_kaapana.py into WrapImage/ directory
Comment thread WrapImage/nifti_wrapper_kaapana.py Outdated
save_nifti_file(D_image, affine, os.path.join(element_output_dir, "d.nii.gz"))

# Copy all .nii.gz from input directory to WORKFLOW_DIR (Kaapana workaround)
nii_files = glob.glob(os.path.join(element_input_dir, "*.nii.gz"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about just saving the output files to this folder directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Due to the sources file bug, the minio put operator is looking for the nifti input fil in the WORKFLOW_DIR that is why I am copying it there. The output file is expected to be saved in the element_output_dir, that is the convension.The above was a workaround due to the bug in kaapana.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really understand. The input files are already in the input folder so it's just about moving the new files to the correct folder?

Comment thread kaapana_ivim_osipi/Workflow_Extension/README.md Outdated
Comment thread kaapana_ivim_osipi/Workflow_Extension/workflow_extension.png Outdated
"description": "Select the algorithm to use.",
"type": "string",
"enum": [
"ASD_MemorialSloanKettering_QAMPER_IVIM",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a way to populate this from the algorithms itself rather than hard coding?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It should be possible, we will just need an interface(function or something) that returns all the algorithms, we can then convert it to json.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@etpeterson, do we currently have any function that return all the algorithms present in our repository ?

# -----------------------------
# Shared IVIM fitting
# -----------------------------
ivim_fitting_task_dicom = MyIvimFittingOperator(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No luck merging earlier?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if you look at the https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/pull/112/files/8372c92a6bc251bd2bf7a77828bb373182769a54#diff-c9a68a5e72bad521effe2518b51fba76337e0e0d8c31dbfdf22d055ec4ecc7d3, we combine it by

trigger_rule="none_failed_or_skipped, we can also do that here but, we would not be able to use theelement_input_dir to get the input data which is an output from the previous dag, we will have figure out where the data from previous operator is stored which means, we will mostly hardcode where the data is coming from.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you're trying to show. I don't see it in Kaapana docs, but in airflow I was suggesting conditional operations. Is that also what you're talking about? https://airflow.apache.org/docs/apache-airflow/1.10.1/concepts.html

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah above, I am using the conditional operation above.

# -----------------------------
# Branch function
# -----------------------------
def choose_upload_type(**context):
    wf_form = context["dag_run"].conf.get("workflow_form", {})
    upload_type = wf_form.get("upload_type", "nifti")
    log.info(f"Upload type selected: {upload_type}")
    if upload_type == "dicom":
        return "get_input"
    return "download-data"

Comment thread WrapImage/nifti_wrapper_kaapana.py
…aapana

- Removed otsus method descrition.
- Add Docs for Kaapana and ivim-fitting-method workflow
 INSTALL.md - for installing Kaapana
 README.md - Description about the workflow and how to install the
 workflow on Kaapan
Add complete documentation covering NIfTI and DICOM workflows,
parameter configuration, and step-by-step usage instructions
Copy link
Copy Markdown
Contributor

@etpeterson etpeterson left a comment

Choose a reason for hiding this comment

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

It's getting there but I'm worried about how many unresolved comments are still here. Are you able to resolve everything in a few days?

Comment thread WrapImage/nifti_wrapper_kaapana.py Outdated
# -----------------------------
# Shared IVIM fitting
# -----------------------------
ivim_fitting_task_dicom = MyIvimFittingOperator(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you're trying to show. I don't see it in Kaapana docs, but in airflow I was suggesting conditional operations. Is that also what you're talking about? https://airflow.apache.org/docs/apache-airflow/1.10.1/concepts.html

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

It's getting there but I'm worried about how many unresolved comments are still here. Are you able to resolve everything in a few days?

Yeah, it should, Since I have the baseline now, I will focus on addressing all the unresolved comments now.

Instead of parsing the sources_file based on order, we now
parsed it based on the file extension.
- Install dcm2niix via curl from GitHub releases instead of Debian package
  to ensure JPEG-LS support
- Remove dcmdjpls decode step and temporary raw_dicoms folder
- Run dcm2niix directly on input directories
- Simplifies workflow and ensures compatibility with JPEG-LS encoded DICOMs
- Ensures each NIfTI in dicom_to_nifti uses correct BVAL/BVEC
- Handles filenames with multiple dots
Copy link
Copy Markdown
Contributor

@etpeterson etpeterson left a comment

Choose a reason for hiding this comment

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

Looks good. I think you fixed the major stuff.

@etpeterson
Copy link
Copy Markdown
Contributor

@oliverchampion Ok to merge this? It should provide a web (kaapana) based instance of the fitting that can run uploaded data. It's independent of the other code so I don't think it impacts anything other than perhaps needing to update it if the standard wrapper changes.

@oliverchampion
Copy link
Copy Markdown
Collaborator

Cool! Is there any way I can see the web-based fitting?

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

Cool! Is there any way I can see the web-based fitting?

While it required setting up a kaapana instance, we can give you access to the instance we are using for development and testing.

@oliverchampion
Copy link
Copy Markdown
Collaborator

If easy, I'm interested in seeing it work. But happy to merge otherwise :). Maybe a short demo Tuesday?

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

If easy, I'm interested in seeing it work. But happy to merge otherwise :). Maybe a short demo Tuesday?

Yeah, I will love to have a demo with you, what time are you chanced on Tuesday ? Also, we can have it merged before then though :)

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

@oliverchampion, calling your attention to this, what time are you chanced on Tuesday ? Thank you.

@oliverchampion
Copy link
Copy Markdown
Collaborator

Hey! We have our regular TF meeting at 15:00 CEST. Maybe you can join and if there is time at the end, give a demo? Otherwise right after the meeting?

1 similar comment
@oliverchampion
Copy link
Copy Markdown
Collaborator

Hey! We have our regular TF meeting at 15:00 CEST. Maybe you can join and if there is time at the end, give a demo? Otherwise right after the meeting?

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

@oliverchampion, that is perfectly okay by me, I will send you an email to get access to the meeting link. Thank you.

@Unique-Usman
Copy link
Copy Markdown
Contributor Author

@oliverchampion bringing your attention to this. Thanks.

@oliverchampion
Copy link
Copy Markdown
Collaborator

@Unique-Usman , my compliments!

@oliverchampion oliverchampion merged commit 8c632cf into OSIPI:main Sep 10, 2025
9 checks passed
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