mne-tools
BUG: incomplete handling of callables
I think the idea is that this local cfg should have everything that is needed to run and get_input_files, and this breaks that convention.
In terms of following this standard, if we do this (use of config directly) in other cases, we risk breaking the caching because joblib won't see any changes in cfg (I think). In this particular case it will still see the changes I think because the input files will change, for the most part. One case that isn't covered is if it's a callable -- now if the code of that callable changes, the caching won't work. (Actually I suspect it will show up as "always new" because by default the __repr__ of a function includes its id/address, which will change on almost every run even if the code doesn't change. But maybe joblib is smarter about handling this already...?)
For now we could mark this with # TODO FIX just to get things green, though, since this seems like a complex problem with no easy solution, and the callable noise_cov case is rare relative to the others. If we do this, we might want to make it always recompute in the case of a callable noise_cov for safety purposes (i.e., we are not smart enough yet to know if someone changes the code within this function).
Originally posted by @larsoner in https://github.com/mne-tools/mne-bids-pipeline/pull/576#discussion_r933802183
From @hoechenberger :
... I believe we're doing the same thing in a few other places where we accept callables, for example I think for that T1 path config option ... But there might be others as well ... Finding a clean solution for all of these would be great.
one possibility when cfg.attr is callable would be to hash the function string content
Message ID: @.***>
This would mean that all functions this function calls will have to be defined or imported inside of the function body too, right?
yes it's not a silver bullet. You could easily break this. Honestly if caching has some failure mode but does the job for 90% of the usecase I am already super happy
Message ID: @.***>
I think we have fixed this in the sense that we detect the callable and prevent any caching for example in the case of cov-as-callable. This pattern should be workable so I'll close