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

Add image_math tool #107

Merged
merged 5 commits into from
Mar 9, 2024
Merged

Conversation

kostrykin
Copy link
Member

@kostrykin kostrykin commented Mar 9, 2024

For the contributor:

  • - I have read the CONTRIBUTING.md document.
  • - License permits unrestricted use (educational + commercial).
  • - This PR adds or updates a tool or tool collection.
  • - This PR does something else (explain below).

Add tool for processing images using arithmetic expressions.

In particular, this tool can be used to negate images (this is what is meant by image inversion in #105 according to the Java source code). An example is given in the help section of the tool.

Squashed commit of the following:

commit 1c8752e
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 11:45:54 2024 +0000

    Fix tool

commit 62e7fe0
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 11:43:09 2024 +0000

    Add more tests

commit 9b76115
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 07:58:26 2024 +0000

    Fix regex

commit 3c91ce8
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:47:23 2024 +0000

    Update help

commit 79ceef3
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 22:47:09 2024 +0100

    Fix tests

commit 6e762c7
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:39:58 2024 +0000

    Add tests

commit 181b0b9
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 22:34:07 2024 +0100

    Add test results

commit 490e713
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:32:50 2024 +0000

    Fix bug

commit 8ca30a3
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:24:08 2024 +0000

    Add tests

commit 321e2f9
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:22:01 2024 +0000

    Fix bug

commit 561c382
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:18:25 2024 +0000

    Fix bug

commit 8ab67a3
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:15:55 2024 +0000

    Fix bug

commit 856f343
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:06:30 2024 +0000

    Fix tool

commit 966aa1f
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:02:04 2024 +0000

    Fix tool

commit ef715ef
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:01:10 2024 +0000

    Update tests

commit edc1b91
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 20:58:44 2024 +0000

    Add `image_math` tool
]]>
</command>
<inputs>
<param argument="--expression" type="text" label="Expression" optional="false">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little bit worried about the security of this tool.

Can you please look at this tool https://github.com/galaxyproject/tools-iuc/blob/main/tools/column_maker/column_maker.xml#L5

And see if you can get some inspirations on how to avoid arbitrary code executions.

For example we could also just allow a few operations abs/sqr etc ... and add more if there is a real need.
The linked tool above also adds the allowed tools to the help, so people know what they can use.

Am I overly careful here?

Copy link
Member Author

@kostrykin kostrykin Mar 9, 2024

Choose a reason for hiding this comment

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

I understand your concerns, I was also a bit concerned at first. However, there also are Jupyter notebooks running on Galaxy, which also permit arbitrary code execution, right? Or are these apples and oranges, somehow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apple and Organes, Jupyter is running only in Docker and is locked down. We also do not allow to run Jupyter 100 times with a lot of resources etc ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, makes sense. Then I will look for some other solution which avoids eval.

Copy link
Member Author

@kostrykin kostrykin Mar 9, 2024

Choose a reason for hiding this comment

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

I have purged the eval-based implementation and replaced it by a custom implementation using abstract syntax trees in 080ffe7. I think this should be safe, because only a small set of operators is permitted, only integer and float literals, and only variables passed in via --input.

And the only allowed function calls are abs and sqrt. I have updated the help section in 01eaf51. The help section now lists all supported operators and functions:

  • Addition, subtraction, multiplication, and division (+, -, *, /)
  • Integer division (e.g., input // 2)
  • Power (e.g., input ** 2)
  • Negation (e.g., -input)
  • Absolute values (e.g., abs(input))
  • Square root (e.g., sqrt(input))
  • Combinations of the above (also using parentheses)

If you prefer that, I think we could leave out Integer division, Power, Absolute values, and Square root for the moment, but I don't think that including these is an issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add all functions you like, I was just anxious about arbitrary eval() executions. My abs/sqr was just a stupid example.

Thanks a lot for changing the implementation so fast.

kostrykin and others added 4 commits March 9, 2024 16:57
Squashed commit of the following:

commit 78edfb2
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 19:56:41 2024 +0000

    Add comments

commit 8bc558a
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 19:52:26 2024 +0000

    Fix bug

commit b6e301c
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 19:29:06 2024 +0000

    Add missing nodes

commit ed2fc08
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 18:35:35 2024 +0000

    Add AST-based implementation

commit 5b9c872
Merge: 1c8752e eda7adf
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 18:22:08 2024 +0000

    Merge branch 'add-image-math-tool/upstream' into add-image-math-tool/dev

commit eda7adf
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 16:57:51 2024 +0100

    Update tools/image_math/image_math.xml

    Co-authored-by: Björn Grüning <[email protected]>

commit 1c8752e
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 11:45:54 2024 +0000

    Fix tool

commit 62e7fe0
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 11:43:09 2024 +0000

    Add more tests

commit 9b76115
Author: Leonid Kostrykin <[email protected]>
Date:   Sat Mar 9 07:58:26 2024 +0000

    Fix regex

commit 3c91ce8
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:47:23 2024 +0000

    Update help

commit 79ceef3
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 22:47:09 2024 +0100

    Fix tests

commit 6e762c7
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:39:58 2024 +0000

    Add tests

commit 181b0b9
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 22:34:07 2024 +0100

    Add test results

commit 490e713
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:32:50 2024 +0000

    Fix bug

commit 8ca30a3
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:24:08 2024 +0000

    Add tests

commit 321e2f9
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:22:01 2024 +0000

    Fix bug

commit 561c382
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:18:25 2024 +0000

    Fix bug

commit 8ab67a3
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:15:55 2024 +0000

    Fix bug

commit 856f343
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:06:30 2024 +0000

    Fix tool

commit 966aa1f
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:02:04 2024 +0000

    Fix tool

commit ef715ef
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 21:01:10 2024 +0000

    Update tests

commit edc1b91
Author: Leonid Kostrykin <[email protected]>
Date:   Fri Mar 8 20:58:44 2024 +0000

    Add `image_math` tool
…alaxy-image-analysis into add-image-math-tool/upstream
Copy link
Collaborator

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

The tool looks great to me. Please decide on your own when you want to merge or add new function.

Thanks!

@kostrykin kostrykin merged commit b356d76 into BMCV:master Mar 9, 2024
10 checks passed
@kostrykin kostrykin deleted the add-image-math-tool/upstream branch March 9, 2024 21:51
kostrykin added a commit that referenced this pull request Mar 10, 2024
Add new tools from #106, #107, #108
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