Skip to content
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

fix: Export samtools wrappers #1306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

msto
Copy link
Contributor

@msto msto commented Sep 10, 2024

pysam does not currently export the various wrapper functions in the samtools module, resulting in type-checking errors.

e.g.

45: error: Module has no attribute "faidx"  [attr-defined]

I've followed the pattern implemented in the other modules, exporting attributes in a module-specific __all__ declaration and then adding this list to the package's __all__.

I believe this PR addresses the problem, but I'm unable to verify - when I install the updated package locally with pip install -e, mypy cannot find the type stubs for pysam. (As an aside - is there a CONTRIBUTING or other documentation for local install/testing?)

Thanks!

@msto msto changed the title Export samtools wrappers fix: Export samtools wrappers Oct 17, 2024
@jmarshall
Copy link
Member

Ideally such documentation would be in doc/developer.rst or would be unnecessary, but neither seems to be the case.

Me, I generally build with python3.xx setup.py build and have /path/to/pysam/build/lib.platform-arch-cpython-3xx listed in a $USER_SITE/localcustomize.pth path configuration file. This is a bit low-tech but suffices to run pytest and — from any directory other than the pysam source directory — to import pysam or run mypy.

With this I could verify whether your PR solves the problem. Unfortunately it does not — __all__ seems to be a red herring here, and to be honest I'm not sure we're using it correctly at all in pysam/__init__.py. What does appear to do the trick is instead adding explicit type hints to pysam/__init__.py, which is probably unsurprising in retrospect:

faidx: pysam.utils.PysamDispatcher
# etc

@jmarshall
Copy link
Member

Personally I'd like to deprecate having the samtools subcommands in the root pysam namespace. The bcftools subcommands need to be used like this:

import pysam.bcftools
pysam.bcftools.view(…)

And the samtools subcommands can be used similarly:

import pysam.samtools
pysam.samtools.view(…)

(And both of the above are type hinted appropriately.)

Additionally at present the following also work for samtools:

import pysam
pysam.samtools.view(…)
pysam.view(…)

Thus the samtools subcommands are available after plain import pysam and also they are available directly as pysam.xyz(…). The latter is redundant and, especially for pysam.view(…) and other subcommands that exist in both samtools and bcftools, has the appearance of ambiguity. So I would like to deprecate that and eventually remove it. But while it's there it should probably be type hinted appropriately nonetheless.

If we also deprecated having import pysam make pysam.samtools.view(…) et al available, then AIUI plain import pysam would not need to load libcsamtools.so, which would be a small speedup and isolation win. Or that may not be worthwhile, in which case we should have plain import pysam make pysam.bcftools.xyz(…) available too.

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.

2 participants