Closed jdblischak closed 1 year ago
omerwe
commented
1 year ago Thanks @jdblischak! I don't have the bandwidth to work on this, so I'll happy merge the PR. Really appreciate your help in keeping PolyFun functional in the jungle of backward-compatability-breaking package updates!
Ojami
commented
1 year ago @jdblischak This fixed the error for me, can you please also check?
snames = (susie_obj.names).tolist()
self.susie_dict = {key: np.array(susie_obj.rx2(key), dtype=object) for key in snames}Also susie_rss has replaced susie_suff_stat
https://github.com/stephenslab/susieR/blob/da24d5fba95845ad082b893f9c80b85b7523c6f6/R/susie_ss.R#L69
jdblischak
commented
1 year ago This fixed the error for me, can you please also check?
@Ojami Thanks for the workaround! It worked with the latest rpy2 (3.5.10), but it's not backwards compatible. It failed with version 3.5.6 with the following error:
Traceback (most recent call last):
File "~/repos/polyfun/finemapper.py", line 1269, in <module>
df_finemap = finemap_obj.finemap(locus_start=args.start, locus_end=args.end, num_causal_snps=args.max_num_causal,
File "~/repos/polyfun/finemapper.py", line 897, in finemap
snames = (susie_obj.names).tolist()
AttributeError: 'StrVector' object has no attribute 'tolist'Ideally we can find a fix that is backwards compatible. Could you please open a new Issue to propose your workaround? Then we can discuss potential improvements in that new issue
jdblischak
commented
1 year ago Also
susie_rsshas replacedsusie_suff_stat
Good point. I see that this happened starting in version 0.12.00 a little over one year ago
https://github.com/stephenslab/susieR/commit/4fe3f1562e25b9c4f8467b118dab5724c0d8b00a
Could you please open a new Issue to discuss updating susieR to >= 0.12? My fine-mapping pipeline currently works fine with susieR 0.11.92, so I don't have a lot of incentive to migrate. Also, @omerwe has made it clear he doesn't currently have the bandwidth for active maintenance.
Without new maintainers to actively lead the project forward, the "polyfun method of fine-mapping" will by necessity imply using the software packages as they existed in the time period 2019-2021. It's one thing to find and merge maintenance fixes due to minor changes in eg numpy/pandas, but it's a lot bigger task to properly evaluate a major interface change to a core fine-mapping algorithm. A maintainer would need to assess the differences between susie_suff_stat() and susie_rss() (especially the impact of default values for arguments like estimate_residual_variance), and also make difficult decisions of when and how to break backwards compatibility.
jdblischak
commented
1 year ago Quick update: finemapper.py can now be run with rpy2 >= 3.5.9 (#156)
Thanks @Ojami for the updated rpy2 code!
tl;dr recent versions of rpy2 break
finemapper.pyProblem
Today I installed a fresh conda env from
polyfun.ymland ranpython test_polyfun.py. It failed when extracting information from the object returned by susieR. I confirmed the tests passed with my old, existing polyfun env, and eventually narrowed the problem down to rpy2.Reproduce the error
Full error message
```python ********************************************************************* * Fine-mapping Wrapper * Version 1.0.0 * (C) 2019-2022 Omer Weissbrod ********************************************************************* [INFO] Loading sumstats file... [INFO] Loaded sumstats for 966 SNPs in 0.53 seconds [INFO] cffi mode is CFFI_MODE.ANY [INFO] R home found: ~/.conda/envs/polyfun-test/lib/R [DEBUG] Looking for LD_LIBRARY_PATH with: ~/.conda/envs/polyfun-test/lib/R/bin/Rscript -e cat(Sys.getenv("LD_LIBRARY_PATH")) [INFO] R library path:`mamba list --explicit -n polyfun-test`
``` # This file may be used to create an environment using: # $ conda create --nameDetermining rpy2 version when problem was introduced
I tried various versions of rpy2. The two latest releases produce the error
The exact line that causes the failure is:
https://github.com/omerwe/polyfun/blob/08c62fb333a07453ec73781bf1223fc5934bc5f0/finemapper.py#L907
As best as I can understand, trying to access an item from an R object inside of this dictionary comprehension fails because it doesn't use the new
.context()syntax introduced in rpy2 3.5.9. I confirmed that aforloop also fails with the same error:And I did my best to emulate their test code, but it also didn't work:
Note that all of the following still work in 3.5.9:
It only fails when the variable changes as part of each iteration (ie the whole point of using the loop):
Proposed solution
I'm not an rpy2 expert, so perhaps you can see a way to adapt the code to use this new setup. Given the complexity of rpy2, especially with numpy, I've left the Python code as is and pinned
rpy2<3.5.7. The reason I chose<3.5.7is because the breaking changes introduced in 3.5.9 were added in order to fix a bug related to converting missing values that was first introduced in 3.5.7. Thus I don't think we should use 3.5.7 or 3.5.8 unless we can convince ourselves that this bug won't apply to polyfun (personally doesn't seem worth the risk to me).References
.context()syntax https://github.com/rpy2/rpy2/pull/993