-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add PySCENIC #50445
base: master
Are you sure you want to change the base?
Add PySCENIC #50445
Conversation
This seems to be often failing (and then losing the logs) with:
on test linux... |
Getting past that and now sort of stuck in:
|
Stuck on the above now for 30 minutes... otherwise all looks good. |
recipes/pyscenic/meta.yaml
Outdated
- url: "https://files.pythonhosted.org/packages/source/a/arboreto/arboreto-0.1.6.tar.gz" | ||
sha256: "32fdac5e8a3e0ef2e196b5827f067d815ac4e689d2fca0dc437f42abdeeb89ab" | ||
folder: arboreto-0.1.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiliyaBioinf arboreto is available from conda, and as such should be used as a conda dependency. Can you remove it from the sources and add under requirements.run please? Can you do the same with any of the above sources that might also be in conda (usually searching for conda + name of the package should allow you to find them if the exists as conda packages). Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and of course, if added to the requirements, then shouldn't go in the build.sh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there was some discussion a while back to add multiprocessing_on_dill to conda-forge , but the upstream maintainers did not approve the PR to add the license. That would need to be resolved in some way so it can be added to conda-forge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe in the meantime multiprocessing_on_dill can be installed through pip so that the community can benefit soon from the pyscenic recipe.
We have had some resource issues, such as running out of memory, with Azure recently. After moving all dependencies out of source (either using existing bioconda or conda-forge packages or by adding new recipes in separate approved/merged PRs), if you are still seeing the Azure linux-64 build hang/disconnect, let me know and I will take a look. |
@BiocondaBot please fetch artifacts |
Thanks @LiliyaBioinf for addressing the issues. |
Package(s) built are ready for inspection:
Docker image(s) built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the recipe, but I don't think this should be packing its dependencies into the recipe as sources. One of the reasons is that license for distribution needs to be accounted for.
recipes/pyscenic/meta.yaml
Outdated
sha256: "{{ sha256 }}" | ||
folder: pyscenic-0.12.1 | ||
|
||
- url: "https://github.com/aertslab/ctxcore/archive/refs/tags/0.2.0.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctxcore
is in bioconda, so you should be able to add that to the dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed!
recipes/pyscenic/meta.yaml
Outdated
- url: "https://github.com/aertslab/ctxcore/archive/refs/tags/0.2.0.tar.gz" | ||
sha256: "a7ebf0f2625641b76a390993e12042e92fff7d0ac242c7fad5e3bff3ff3cd67a" | ||
folder: ctxcore-0.2.0 | ||
- url: "https://files.pythonhosted.org/packages/source/m/multiprocessing_on_dill/multiprocessing_on_dill-3.5.0a4.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above, the licensing for multiprocessing_on_dill
has to be worked out. It should be added to conda-forge
rather than bioconda because it's not bioinformatics related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working out a license like that could take a long time, in the meantime this could be used like this and possibly improved once that is resolved. There are many packages in bioconda that recourse to pip installing dependencies that are not available in conda, for instance https://github.com/bioconda/bioconda-recipes/blob/def3a07b68330abb40f7650b3747b8bee189ab66/recipes/latch/build.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I see your point. I will get some clarity on this from the core team.
recipes/pyscenic/meta.yaml
Outdated
|
||
extra: | ||
recipe-maintainers: | ||
- 179018299 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this number refer to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was my github id, deleted it
I suspect that you will need multiprocessing_on_dill from source, unless that someone has added it to conda. Thanks for switching ctxcore @LiliyaBioinf . |
Chiming in here, to summarize it sounds like The right way to fix this is to 1) submit a new conda-forge PR and 2) contact the upstream author(s) again. (Side note, the abovementioned latch recipe is the only instance I know of that is installing separately from pip, and is probably a fluke -- my guess is that it got missed in the review process -- and should be fixed.) |
License aside, I have to say I have seen many recipes doing the pip trick when there are no conda packages along the years, here is another one I found quickly https://github.com/bioconda/bioconda-recipes/blob/master/recipes/iphop/build.sh . I think it puts a large burden on contributors to having to create recipes recursively like this. What happens if multiprocessing_on_dill has 2 other deps that are not in conda? Are we asking the contributor to also have to deal with that? where do we stop? |
@pcm32 after discussing with the @bioconda/core team, the consensus is that pip should not be used to install additional packages from within recipes. The existence of existing recipes that do this is an oversight, so we will be fixing them. We will also update the docs to clarify this policy. Regarding recursively adding recipes -- yes, that is exactly what we need to do. If |
Describe your pull request here
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add label
command.@bioconda/core
in a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports
(see here for the rationale and comprehensive explanation).Add a
run_exports
section like this:with
...
being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}
{{ pin_subpackage("myrecipe", max_pin="x.x") }}
{{ pin_subpackage("myrecipe", max_pin="x.x") }}
(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}
(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}
while replacing
"myrecipe"
with eithername
if aname|lower
variable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update
@BiocondaBot please add label
please review & merge
label.@BiocondaBot please fetch artifacts
You can use this to test packages locally.
Note that the
@BiocondaBot please merge
command is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>
and will automatically re-post them to notify the addressed<team>
.