Feature/check minknow #351

mattloose · 2024-04-24T15:52:39Z

This pull request is to address #349 and is raised for discussiong.

Adoni5 · 2024-04-25T08:10:55Z

i love a good discussiong

Adoni5 · 2024-04-25T08:11:25Z

I'm going to change the target for this to feature/dorado

mattloose · 2024-04-25T08:13:05Z

i love a good discussiong

discussiong: noun
the action or process of talking about something in order to reach a conclusion which may take a very long time.

mattloose · 2024-04-25T08:13:39Z

I'm going to change the target for this to feature/dorado

sorry - my bad - yep please do.

Adoni5

I've moved this to merge into the feature/dorado branch - as it has all the commits from that branch as part of it's history!

Is that correct?

src/readfish/plugins/dorado.py

src/readfish/plugins/guppy.py

src/readfish/__about__.py

src/readfish/_utils.py

src/readfish/__compatability__.py

Adoni5 · 2024-04-25T08:53:53Z

And I think you need to add packaging as a dependency!

Adoni5 · 2024-04-25T09:07:28Z

And I think you need to add packaging as a dependency!

I see that it is a dependency of the minknow_api, but maybe we could explicitly add it as a dependency anyway? Explicit is better than implicit after all

mattloose · 2024-04-26T13:40:19Z

I've moved this to merge into the feature/dorado branch - as it has all the commits from that branch as part of it's history!

Is that correct?

probably

Base-caller compatibiity check

…longer raiseException when a version issue is encountered. This means that readfish will continue to function with future versions of minKNOW if compatible.

Prevents `KeyError` from being raised if `sample_rate`isn't listed as a `kwarg`

We now just check in `dorado.py` and warn if there is a mismatch

Adoni5

LGTM - @alexomics what say you?

alexomics

Looks good! Only comments are:

the _compatibility.DIRECTION enum is a little opaque on first reading. It's not clear if that's saying upgrade readfish or MinKNOW.
check_compatibility looks hard to use in the code. Does it need to return a tuple/could the check be specifically for an actionable DIRECTION variant? E.g:

if (action := check_compat(...)) in (DIRECTION.UP, DIRECTION.DOWN):
    log(action)

Adoni5 · 2024-05-07T15:25:34Z

* the `_compatibility.DIRECTION` enum is a little opaque on first reading. It's not clear if that's saying upgrade readfish or MinKNOW.

readfish/src/readfish/_compatibility.py

Lines 29 to 31 in dc6644c

    
           class DIRECTION(Enum): 
        
               """ 
        
               Upgrade, downgrade or just right

Amazing stuff, I don't know how it could be clearer 👾
( Definitely thought that this docstring actually had contents)

* `check_compatibility` looks hard to use in the code. Does it need to return a tuple/could the check be specifically for an actionable `DIRECTION` variant? E.g:

if (action := check_compat(...)) in (DIRECTION.UP, DIRECTION.DOWN):
    log(action)

Yeah agreed, it should just return the Enum variant or None not the tuple, makes more sense 👍🏼

Only return Enum variant from `check_compatibility`, not tuple of (bool, variant)

alexomics · 2024-05-07T16:20:33Z

LGTM! 🚀

mattloose · 2024-05-07T17:42:21Z

Much awesomeness.

* Introducing a dorado specific plugin which handles the change from ont_pyguppy_client_lib to ont_pybasecaller_client_lib * minor code reformat * Bump version * Update Changelog * Add `ont-pybasecall-client-lib` as a dependency under target `dorado` * Add try/except on import helper functions and pyclient for basecaller This is so pytest collections (The only time both are imported into the same interperter runtime) doesn't crash. This requires all tests to only test one, so I've moved all tests to test dorado * Move tests to test dorado rather than guppy plugin when validating This is due to the fact that pytest imports all files, the only time that pyguppy-client-lib and pybasecall-client-lib are imported at the same time In a real experiment, the plugin choice for base-caller imports the correct module and leaves the other one out of the interpreter runtime * Remove dorado, guppy and _read_until_client from coverage reporting as we can't really cover them They require things link a read_until_api or live base caller to properly test * Introducing sample rate from minknow and catching sub_read issue. * Updating documentation for dorado * Feature/check minknow (#351) * Initial validation of minknow version and guppy dorado versions * MinKNOW compatibility check function Base-caller compatibiity check * edit insanely long string multiline string example of sub tags * Local updates * Error checking and minor corrections to address compatibility. We no longer raise RuntimeException when a version issue is encountered. This means that readfish will continue to function with future versions of minKNOW if compatible. * Add default when popping `sample_rate` from guppy params Prevents `KeyError` from being raised if `sample_rate`isn't listed as a `kwarg` * Remove unused basecaller compatibilty function We now just check in `dorado.py` and warn if there is a mismatch * Correct docstring, add doctests * Add more complete docstring to `DIRECTION` enum Only return Enum variant from `check_compatibility`, not tuple of (bool, variant) * Suggest opening an issue if a suitable version of readfish doesn't exist --------- Co-authored-by: Adoni5 <roryjmunro1@gmail.com> * Add __futures__ annotation for Py38 compatibility * Deprecation warning on Guppy plugin * Add check for Write permission on Dorado socket --------- Co-authored-by: Adoni5 <roryjmunro1@gmail.com>

Initial validation of minknow version and guppy dorado versions

b304eb8

Adoni5 changed the base branch from main to feature/update_pybasecall_client_lib April 25, 2024 08:11

Adoni5 requested changes Apr 25, 2024

View reviewed changes

Adoni5 and others added 10 commits April 29, 2024 14:05

MinKNOW compatibility check function

40760a8

Base-caller compatibiity check

Compatibility checks

4298da9

Remove rogue comment

246e69c

edit insanely long string multiline string example fo sub tags

46a3d9e

Local updates

d46b53b

Error checking and minor corrections to address compatibility. We no …

69b2c75

…longer raiseException when a version issue is encountered. This means that readfish will continue to function with future versions of minKNOW if compatible.

Error checking and minor corrections to address compatibility. We no …

3571012

…longer raiseException when a version issue is encountered. This means that readfish will continue to function with future versions of minKNOW if compatible.

Add default when popping sample_rate from guppy params

d56042a

Prevents `KeyError` from being raised if `sample_rate`isn't listed as a `kwarg`

Remove unused basecaller compatibilty function

6cb85e3

We now just check in `dorado.py` and warn if there is a mismatch

Correct docstring, add doctests

dc6644c

Adoni5 approved these changes May 7, 2024

View reviewed changes

alexomics approved these changes May 7, 2024

View reviewed changes

Adoni5 added 2 commits May 7, 2024 16:51

Add more complete docstring to DIRECTION enum

aaebfca

Only return Enum variant from `check_compatibility`, not tuple of (bool, variant)

Suggest opening an issue if a suitable version of readfish doesn't exist

8fec91e

Adoni5 merged commit 3482d59 into feature/update_pybasecall_client_lib May 7, 2024

Adoni5 deleted the feature/check_minknow branch May 7, 2024 16:58

Adoni5 mentioned this pull request Jun 19, 2024

Detect MinKNOW version and warn. #349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/check minknow #351

Feature/check minknow #351

mattloose commented Apr 24, 2024

Adoni5 commented Apr 25, 2024

Adoni5 commented Apr 25, 2024

mattloose commented Apr 25, 2024

mattloose commented Apr 25, 2024

Adoni5 left a comment

Adoni5 commented Apr 25, 2024

Adoni5 commented Apr 25, 2024

mattloose commented Apr 26, 2024

Adoni5 left a comment

alexomics left a comment

Adoni5 commented May 7, 2024

alexomics commented May 7, 2024

mattloose commented May 7, 2024

Feature/check minknow #351

Feature/check minknow #351

Conversation

mattloose commented Apr 24, 2024

Adoni5 commented Apr 25, 2024

Adoni5 commented Apr 25, 2024

mattloose commented Apr 25, 2024

mattloose commented Apr 25, 2024

Adoni5 left a comment

Choose a reason for hiding this comment

Adoni5 commented Apr 25, 2024

Adoni5 commented Apr 25, 2024

mattloose commented Apr 26, 2024

Adoni5 left a comment

Choose a reason for hiding this comment

alexomics left a comment

Choose a reason for hiding this comment

Adoni5 commented May 7, 2024

alexomics commented May 7, 2024

mattloose commented May 7, 2024