MagneticResonanceImaging / MRIReco.jl

Julia Package for MRI Reconstruction
https://magneticresonanceimaging.github.io/MRIReco.jl/latest/
Other
85 stars 22 forks source link

MRIReco v0.8.0 incorrect initialization with multiple threads #178

Closed beorostica closed 5 months ago

beorostica commented 6 months ago

I noticed that MRIReco v0.8.0 cannot be properly initialized with multiple threads. This is a MWE:

~$ julia --threads=auto

julia> using MRIReco
ERROR: InitError: UndefVarError: `FFTW` not defined
Stacktrace:
  [1] __init__()
    @ MRIReco ~/.julia/packages/MRIReco/rKbDs/src/MRIReco.jl:23
  [2] run_module_init(mod::Module, i::Int64)
    @ Base ./loading.jl:1128
  [3] register_restored_modules(sv::Core.SimpleVector, pkg::Base.PkgId, path::String)
    @ Base ./loading.jl:1116
  [4] _include_from_serialized(pkg::Base.PkgId, path::String, ocachepath::String, depmods::Vector{Any})
    @ Base ./loading.jl:1061
  [5] _require_search_from_serialized(pkg::Base.PkgId, sourcepath::String, build_id::UInt128)
    @ Base ./loading.jl:1575
  [6] _require(pkg::Base.PkgId, env::String)
...

It looks like, in the commit https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/291884f5d8e48f5fa1585d551715951cef7d69d9 you already addressed this issue, however this would be reflected in the version 0.9.0 of MRIReco.

Is it possible to create a patch release 0.8.1 of MRIReco with this small change?

aTrotier commented 6 months ago

A workaround is to load the package twice

using MRIReco using MRIReco

Le jeu. 18 janv. 2024 à 21:33, beorostica @.***> a écrit :

I noticed that MRIReco v0.8.0 cannot be used with multiple-threads. This is the MWE:

~$ julia --threads=auto

julia> using MRIReco ERROR: InitError: UndefVarError: FFTW not defined Stacktrace: [1] init() @ MRIReco ~/.julia/packages/MRIReco/rKbDs/src/MRIReco.jl:23 [2] run_module_init(mod::Module, i::Int64) @ Base ./loading.jl:1128 [3] register_restored_modules(sv::Core.SimpleVector, pkg::Base.PkgId, path::String) @ Base ./loading.jl:1116 [4] _include_from_serialized(pkg::Base.PkgId, path::String, ocachepath::String, depmods::Vector{Any}) @ Base ./loading.jl:1061 [5] _require_search_from_serialized(pkg::Base.PkgId, sourcepath::String, build_id::UInt128) @ Base ./loading.jl:1575 [6] _require(pkg::Base.PkgId, env::String)...

It looks like, in the commit 291884f https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/291884f5d8e48f5fa1585d551715951cef7d69d9 you already address this issue, however this would be reflected in the version 0.9.0 of MRIReco.

Is it possible to create a patch release 0.8.1 of MRIReco with this small change?

— Reply to this email directly, view it on GitHub https://github.com/MagneticResonanceImaging/MRIReco.jl/issues/178, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5P7O656TGIDLZQLKW3PWLYPGBLDAVCNFSM6AAAAABCA4ROFOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA4DSMBQHA3DANY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

TROTIER Aurélien 31 allée de la Biotte 33470 Gujan-Mestras 06.09.12.61.61 @.***

beorostica commented 6 months ago

Thanks for pointing that out, @aTrotier.

However, our KomaMRI.jl package is experiencing the same initialization issue because MRIReco is part of its dependencies, and we don't want to give a bad impression to our new users.

We hope that this makes sense to you too, and even though this may add more workload to your team, we would really appreciate it if you could do something on the matter.

aTrotier commented 5 months ago

It also is an issue for one of my package.

I suppose we have to change the git history to put the fix right after the v0.8.0 because some breaking changes with RegularizedLeastSquares has been implemented later.

Does moving this commit https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/291884f5d8e48f5fa1585d551715951cef7d69d9 after this one c259559ecc715cb6415fce786833dfbce59506f4 will be ok ?

beorostica commented 5 months ago

I think it is safer to apply the change right after the commit of the version 0.8.0 https://github.com/MagneticResonanceImaging/MRIReco.jl/commit/8307c24a6086d202fe8328f0727548de54ec8137

tknopp commented 5 months ago

Done in https://github.com/JuliaRegistries/General/pull/99190

aTrotier commented 5 months ago

It seems you have made the release after some breaking changes about regls, no ?

Can we first rebase and move the commit that fix FFT right after v0.8.0 for the v0.8.1.

The release after updating the regls interface should be 0.9.0

cncastillo commented 5 months ago

Thank you @tknopp !! version v0.8.1 is not generating any problems on our parts, but I just tested a very basic recon.

@aTrotier what are the breaking changes in RegularizedLeastSquares? So I can test.

I would agree that it makes sense to not introduce breaking changes for a patch version, but I am not good enough with git to fix the commit history.

tknopp commented 5 months ago

I branched of from the last release and just included the single change: see https://github.com/MagneticResonanceImaging/MRIReco.jl/commits/v0.8.1/

aTrotier commented 5 months ago

my bad the release note is wrong :)

https://github.com/MagneticResonanceImaging/MRIReco.jl/releases/tag/v0.8.1

thanks for the release