-
Notifications
You must be signed in to change notification settings - Fork 5
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
Next release #147
Comments
next steps:
|
Thanks, I just merged dieterich-lab/pbiotools#17. I will now merge to master and push. |
@lkeegan what about dieterich-lab/pbiotools#13 |
you can ignore it or merge it (probably easiest to first push your changes) - this PR isn't important and won't break anything, it's just updating the pre-commit version hooks |
No, actually, this doesn't make sense to not release a new version, as the PyPI release needs to change accordingly. If we remove Python 3.6, in particular, then I guess we have to go for v4. |
Ok, pbiotools is now to v4 release. I will try to first sort the docs, in particular #146 , then add a pypi CI to rp-bp (copied from pbiotools), and update the pbiotools v4 requirement, etc. |
@lkeegan , for the PyPI CI (pypi.yml), could you please set-up a template? I thought I'd use the pbiotools one, but in fact we would need to run the conda install, and not the pip install, or I am wrong? |
@eboileau you should be able to use the pbiotools one - this is for PyPI so pip install with pypi dependencies is what we want - this is completely independent of conda/bioconda (although I guess you'll need to also install star etc in the CI if you want to run the tests) |
Would that work to create a conda env to install |
I would suggest to keep it simple, either
In either case, |
Yes, I also think skipping the tests is fine, so this would look like:
|
yes, looks good to me |
Thanks. I will then add this, update reqs to |
Sounds good! I have a wip bioconda recipe here that I'll update once there is a new rp-bp github release: lkeegan/bioconda-recipes@1f19d50 |
Hi @lkeegan |
@eboileau great, I've updated the recipe & made a PR If you download & extract the LinuxArtifacts file here you should be able to both install the conda package & try using the docker image: bioconda/bioconda-recipes#39210 (comment) Let me know if they work for you and I'll ask for the PR to be merged |
Great, I'll test the package and the container usage, and let you know as soon as possible. |
fyi after following the docker instructions there I could then run the docker container with:
Both for conda and docker running |
Thanks. I will try running the pipeline. I also want to try passing the config and writing results to disk, e.g. with |
I have not been able to test the full pipeline. There are two immediate problems:
This is due to
This is happening with Docker, Singularity, and also with the conda install.
This is not a critical issue, but depending on 1 above, we might have to do something anyway... With
but using Do you know of any workaround in Singularity/Docker? Otherwise, we need to modify the code (symlinks should in fact just be relative). |
This seems related. |
1The pre-compiled models link to
But the conda environment instead provides
I'll look into this, we need to modify the recipe somehow to ensure the same tbb library version is being used for compiling and for the user installation. 2I don't know of a good workaround for the symlinks with docker issue - but if you could modify the code to use relative paths to some base path instead of symlinks that would (imo) be a big improvement in usability/portability (not just for Docker) |
It seems the libtbb.so culprit is
Not sure what the best solution is in the meantime, but maybe a pragmatic option would be to skip the pre-compiled models in the first bioconda release and add this in a later bioconda release? |
Hi @lkeegan yes, Flexbar is definitely required. Re 2 above, I don't want to re-write the complete I/O. The easiest solution for now is to remove symlinks by actually renaming files if needed. I'm working on this. |
This case was not set-up in the regression tests as we don't have data for it... this is unfortunate... In the rpbp recipe as well as in toml and environment, we specify @lkeegan can you please bear with me before updating the recipe? Thanks. |
@eboileau yes no problem, I'll try to make some progress on the flexbar issue in the meantime |
I'll be ready to push and make a patch release of Rp-Bp, as soon as pbiotools 4.0.1 is available on bioconda. P.S. I'm off for 2 weeks from 14 Feb. |
Thanks, indeed saw it yesterday. I'll try to sort #150, then release. |
Oh no... For macOS, this doesn't work anymore...
and a lot of warnings... see https://github.com/dieterich-lab/rp-bp/actions/runs/4163364277/jobs/7203670504 |
This looks like a mismatch between compiler & linker. Looking at the conda environment there is a mix of defaults & conda-forge packages which is likely the cause of this - certain packages (like the linker) are not compatible between defaults and conda-forge. The good news is I think this is just an issue with the conda environment on the CI, the first thing I would try is modifying this line: rp-bp/.github/workflows/ci.yml Line 31 in 9f6ec64
Then hopefully the conda-forge packages will take precedence over the defaults. The bad news here is that looking through the logs I noticed that stan is enabling a bunch of cpu-specific optimizations in the c++ compile step, which means the pre-compiled models may crash with "illegal instruction" errors if the cpu used when building the bioconda recipe supports some instruction that the users cpu doesn't.
Maybe option 1 is the safer/simpler way to go, especially as compiling the models doesn't take very long? |
@lkeegan Thanks for your quick feedback. Yes, I was looking into this in more details, I also think setting the channel priority might work. As for the |
I will update the CI config, and remove the Stan cpp option from defaults, and check if tests are passing on GA. |
Damn it! Looks like it's not going to be solved before I leave for vacation... Now it's not resolving Dash + werkzeug versions correctly... |
As this looks like only a problem with the CI setup, maybe it would make sense for you to make a release ignoring the broken CI? I can then do the bioconda packaging & fix the CI, and if there are any issues with the actual release they can be fixed in a patch release when you're back from vacation? Re. shipping pre-compiled models on bioconda after some more research I actually think this should be ok:
|
Wow! But shall I leave |
I think it's ok - this only affects the conda environment in the CI jobs, not what you release |
v3.0.1 is there now. |
This should fix the conda issues in the CI: #151 |
Test suite now runs successfully for all linux conda packages built here as well as one of the docker containers (all with pre-compiled models): bioconda/bioconda-recipes#39210 |
Thank you for this @lkeegan. |
|
I would mostly be ready for the next release, but before
I'd like this to be sorted: the pbiotools CI (remove Python3.6), and update the bioconda recipe (add missing joblib, numpy, pandas, scipy, remove pybedtools and appdirs). I think we can also remove "parse_meme_names.py" and the pyyaml dependency altogether.
what do we do with https://github.com/dieterich-lab/rp-bp/security/dependabot and [pre-commit.ci] pre-commit autoupdate pbiotools#13 ?
Rp-Bp docs, see Need help with docs #146
After that, what remains to be done? i.e. what are the next steps now?
I think we also need to change
requires-python = ">=3.6,<3.11"
torequires-python = ">=3.7,<3.11"
. Do we need to update environment.yml and dependencies in pyproject.toml, etc.I'd also like to get/update some badges, at least for
version
orrelease
,python
,docs
,build
(or CI?),codecov
, andpre-commit
.The text was updated successfully, but these errors were encountered: