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

ENH: Bump elastix to 5.2.0, update wasm #296

Merged
merged 2 commits into from
Aug 12, 2024
Merged

ENH: Bump elastix to 5.2.0, update wasm #296

merged 2 commits into from
Aug 12, 2024

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Jul 19, 2024

Rebase wasm patches for elastix. Bump itk-wasm and the ITK-Wasm docker image for ITK 5.4.0 support. Disable a wasi test with an odd error:

3: Test command: /wasi-runtimes/wasmtime/bin/wasmtime-pwd.sh "/work/wasi-build/elastix.wasi.wasm" "/work/wasm/test/data/input/parameters_single.json" "/work/wasi-build/wasm/CT_2D_head_registered_initial.iwi.cbor" "/work/wasi-build/wasm/CT_2D_head_initial.h5" "/work/wasi-build/wasm/CT_2D_head_initial.json" "-f" "/work/wasm/test/data/input/CT_2D_head_fixed.iwi.cbor" "-m" "/work/wasm/test/data/input/CT_2D_head_moving.iwi.cbor" "-i" "/work/wasm/test/data/input/CT_2D_head_translation.h5" 3: Working Directory: /work/wasi-build/wasm
3: Test timeout computed to be: 10000000
3: Error: failed to run main module /work/wasi-build/elastix.wasi.wasm 3:
3: Caused by:
3: 0: failed to invoke command default
3: 1: error while executing at wasm backtrace:
3: 0: 0x388ff6 - elastix.wasi.wasm!__cxx_global_array_dtor.54
3: 1: 0x95b08 - elastix.wasi.wasm!__funcs_on_exit
3: 2: 0x95cb2 - elastix.wasi.wasm!__wasm_call_dtors
3: 3: 0x15df748 - elastix.wasi.wasm!itk_wasm_delayed_exit
3: 4: 0x15df7d1 - elastix.wasi.wasm!_start
3: note: using the WASMTIME_BACKTRACE_DETAILS=1 environment variable may show more debugging information
3: 2: wasm trap: uninitialized element
3/10 Test #3: elastix-wasm-2d-initial-transform-test ....................***Failed 7.99 sec

@thewtex thewtex requested a review from N-Dekker July 19, 2024 03:34
@N-Dekker
Copy link
Collaborator

Thanks for upgrading to elastix 5.2.0 so quickly after the release (https://github.com/SuperElastix/elastix/releases/tag/5.2.0), Matt!

I'm sorry I feel I cannot review your pull request as a whole, as it involves 700+ lines of code, many of them being beyond my expertise 🤷

@N-Dekker
Copy link
Collaborator

I wonder, would it not be possible to have a separate commit that only just upgrades elastix, without all those other changes? (One Commit. One Change) Wouldn't such a commit pass the tests? If so, can you please explain why not?

@thewtex thewtex marked this pull request as draft July 19, 2024 12:29
@thewtex
Copy link
Member Author

thewtex commented Jul 19, 2024

I wonder, would it not be possible to have a separate commit that only just upgrades elastix, without all those other changes? (One Commit. One Change) Wouldn't such a commit pass the tests? If so, can you please explain why not?

Yes, we have one commit that bumps the elastix hash and the itk-wasm version. They come together for a working commit / update. elastix 5.2.0 requires itk 5.4.0 and this requires an itk-wasm bump.

@thewtex thewtex marked this pull request as ready for review July 19, 2024 19:52
@thewtex thewtex force-pushed the elastix-5.2.0 branch 4 times, most recently from e890ba2 to 40022a7 Compare July 19, 2024 20:38
@N-Dekker
Copy link
Collaborator

Yes, we have one commit that bumps the elastix hash and the itk-wasm version. They come together for a working commit / update. elastix 5.2.0 requires itk 5.4.0 and this requires an itk-wasm bump.

Thanks for explaining @thewtex But ITKElastix was already upgraded to itk 5.4.0 two month ago, right?

Didn't that already require an itk-wasm bump?

@thewtex
Copy link
Member Author

thewtex commented Jul 23, 2024

@N-Dekker the bump was required for itk::Deref.

@N-Dekker
Copy link
Collaborator

the bump was required for itk::Deref.

Thanks Matt. Can you possibly explain why? itk::Deref is very much a C++ utility, it cannot be wrapped to Python. The elastix core library only uses itk::Deref internally, so it's not part of its interface.

@thewtex
Copy link
Member Author

thewtex commented Jul 23, 2024

@N-Dekker we do need to build elastix's core interface regardless of whether it is exposed in Python.

@N-Dekker
Copy link
Collaborator

Thanks @thewtex Still I feel I'm unable to review all of those code changes carefully (+8421 lines, −6,012 lines of code). I'm sorry. 🤷 Maybe someone else can still have a look?

@thewtex
Copy link
Member Author

thewtex commented Jul 24, 2024

@N-Dekker that's ok, thanks for the effort.

I still need to do a bit of work to get the tests passing.

Rebase wasm patches for elastix. Bump itk-wasm and the ITK-Wasm docker
image for ITK 5.4.0 support. Disable a wasi test with an odd error:

```
3: Test command: /wasi-runtimes/wasmtime/bin/wasmtime-pwd.sh "/work/wasi-build/elastix.wasi.wasm" "/work/wasm/test/data/input/parameters_single.json" "/work/wasi-build/wasm/CT_2D_head_registered_initial.iwi.cbor" "/work/wasi-build/wasm/CT_2D_head_initial.h5" "/work/wasi-build/wasm/CT_2D_head_initial.json" "-f" "/work/wasm/test/data/input/CT_2D_head_fixed.iwi.cbor" "-m" "/work/wasm/test/data/input/CT_2D_head_moving.iwi.cbor" "-i" "/work/wasm/test/data/input/CT_2D_head_translation.h5"
3: Working Directory: /work/wasi-build/wasm
3: Test timeout computed to be: 10000000
3: Error: failed to run main module `/work/wasi-build/elastix.wasi.wasm`
3:
3: Caused by:
3:     0: failed to invoke command default
3:     1: error while executing at wasm backtrace:
3:            0: 0x388ff6 - elastix.wasi.wasm!__cxx_global_array_dtor.54
3:            1: 0x95b08 - elastix.wasi.wasm!__funcs_on_exit
3:            2: 0x95cb2 - elastix.wasi.wasm!__wasm_call_dtors
3:            3: 0x15df748 - elastix.wasi.wasm!itk_wasm_delayed_exit
3:            4: 0x15df7d1 - elastix.wasi.wasm!_start
3:        note: using the `WASMTIME_BACKTRACE_DETAILS=1` environment variable may show more debugging information
3:     2: wasm trap: uninitialized element
 3/10 Test  #3: elastix-wasm-2d-initial-transform-test ....................***Failed    7.99 sec
```

Also update CI configuration accordingly.
For reproducible builds.
@thewtex thewtex merged commit 24144dd into main Aug 12, 2024
36 of 38 checks passed
@thewtex thewtex deleted the elastix-5.2.0 branch August 12, 2024 11:10
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.

None yet

2 participants