Skip to content

Conversation

benjeffery
Copy link
Contributor

No description provided.

@benjeffery
Copy link
Contributor Author

benjeffery commented May 13, 2025

I've made all the format-specifc deps including cyvcf2 optional.
Needs some concrete CI tests for the functionality.

@jeromekelleher
Copy link
Contributor

Nice - so the idea is that if you just need to convert plink, you don't have to install cyvcf2? Infrastructure LGTM.

I have some vague recollection of there being a recent PEP on handling this sort of thing, but haven't managed to track it down. @tomwhite, do you know what current best practices on handling optional deps is?

@benjeffery
Copy link
Contributor Author

so the idea is that if you just need to convert plink, you don't have to install cyvcf2

Exactly - cyvcf2 isn't an "easy" dep so seemed wise.

Maybe you are thinking of https://peps.python.org/pep-0631/ ??

@tomwhite
Copy link
Contributor

I have some vague recollection of there being a recent PEP on handling this sort of thing, but haven't managed to track it down. @tomwhite, do you know what current best practices on handling optional deps is?

Possibly this one? https://peps.python.org/pep-0771/

Optional packages can cause problems with conda (as in conda doesn't have a way to install them), but I'm not sure what the best practices there are.

@benjeffery
Copy link
Contributor Author

As far as I'm aware there in conda-land the "best" option is to have several packages e.g. bio2zarr-all bio2zarr-plink ... but that isn't great. Prob best just to have one big build.

@jeromekelleher
Copy link
Contributor

Optional packages can cause problems with conda (as in conda doesn't have a way to install them), but I'm not sure what the best practices there are.

I think you just have to do one big build in conda - I guess the good thing there is that you find out whether that's actually possible as you're building the package. Let's just build for pip for now, and worry about conda later.

@jeromekelleher
Copy link
Contributor

Aha, yes, thanks Tom - it was pep-771. So, we'd default to installing the VCF version if someone requests bio2zarr without any extra specifiers. Is that possible here?

@tomwhite
Copy link
Contributor

Aha, yes, thanks Tom - it was pep-771. So, we'd default to installing the VCF version if someone requests bio2zarr without any extra specifiers. Is that possible here?

I'm not sure it is until 771 is adopted - I think we fall into https://peps.python.org/pep-0771/#packages-supporting-multiple-backends-or-frontends, where "installing the package without any extras results in a broken installation". So for now we just have to document the installation process clearly.

@benjeffery benjeffery force-pushed the optional-deps branch 3 times, most recently from 3dd0fa4 to a52a052 Compare May 14, 2025 10:08
@coveralls
Copy link
Collaborator

coveralls commented May 14, 2025

Coverage Status

coverage: 98.166% (+0.01%) from 98.153%
when pulling 06f8e30 on benjeffery:optional-deps
into 587a29e on sgkit-dev:main.

@benjeffery benjeffery force-pushed the optional-deps branch 2 times, most recently from c3355e3 to 615fba3 Compare May 14, 2025 10:19
Copy link
Contributor

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -21,12 +21,7 @@ dependencies = [
"tabulate",
"tqdm",
"humanfriendly",
# cyvcf2 also pulls in coloredlogs and click",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the comments here with the rationale for these dependencies. The only reason we're using coloredlogs is because it comes anyway with cyvcf2

@benjeffery benjeffery changed the title WIP - Optional deps Optional deps May 14, 2025
@benjeffery benjeffery marked this pull request as ready for review May 14, 2025 10:57
@benjeffery benjeffery enabled auto-merge May 14, 2025 10:57
@benjeffery
Copy link
Contributor Author

I've added the new CI job to the required merge tests.

@benjeffery benjeffery added this pull request to the merge queue May 14, 2025
Merged via the queue into sgkit-dev:main with commit 7942838 May 14, 2025
15 checks passed
@benjeffery benjeffery deleted the optional-deps branch May 14, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants