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 Rust panic error in signature creation #1166

Closed
ctb opened this issue Aug 14, 2020 · 4 comments · Fixed by #1172
Closed

fix Rust panic error in signature creation #1166

ctb opened this issue Aug 14, 2020 · 4 comments · Fixed by #1172
Labels

Comments

@ctb
Copy link
Contributor

ctb commented Aug 14, 2020

The following error was triggered by this command line, in #1159 -

sourmash sketch protein -p k=21,dna tests/test-data/ecoli.faa -o foo.sig

error:

== This is sourmash version 3.5.0a0. ==
== Please cite Brown and Irber (2016), doi:10.21105/joss.00027. ==

computing signatures for files: tests/test-data/ecoli.faa
Computing a total of 1 signature(s).
... reading sequences from tests/test-data/ecoli.faa
Traceback (most recent call last):
  File "/Users/t/miniconda3/envs/py37/bin/sourmash", line 11, in <module>
    load_entry_point('sourmash', 'console_scripts', 'sourmash')()
  File "/Users/t/dev/sourmash/sourmash/__main__.py", line 13, in main
    return mainmethod(args)
  File "/Users/t/dev/sourmash/sourmash/cli/sketch/protein.py", line 69, in main
    return sourmash.command_sketch.protein(args)
  File "/Users/t/dev/sourmash/sourmash/command_sketch.py", line 220, in protein
    _execute_sketch(args, signatures_factory)
  File "/Users/t/dev/sourmash/sourmash/command_sketch.py", line 175, in _execute_sketch
    _compute_individual(args, signatures_factory)
  File "/Users/t/dev/sourmash/sourmash/command_compute.py", line 228, in _compute_individual
    args.input_is_protein, args.check_sequence)
  File "/Users/t/dev/sourmash/sourmash/command_compute.py", line 281, in add_seq
    sig.add_protein(seq)
  File "/Users/t/dev/sourmash/sourmash/signature.py", line 155, in add_protein
    self._methodcall(lib.signature_add_protein, to_bytes(sequence))
  File "/Users/t/dev/sourmash/sourmash/utils.py", line 25, in _methodcall
    return rustcall(func, self._get_objptr(), *args)
  File "/Users/t/dev/sourmash/sourmash/utils.py", line 78, in rustcall
    raise exc
sourmash.exceptions.Panic: sourmash panicked: thread 'unnamed' panicked with 'called `Result::unwrap()` on an `Err` value: InvalidHashFunction { function: "dna" }' at src/core/src/signature.rs:367

there is now a check in the CLI code to prevent this, and a test, but that can be bypassed by commenting out the Incompatible sketch type ValueError exceptions in command_sketch.py if we want to reproduce this.

see @luizirber comment here

@ctb
Copy link
Contributor Author

ctb commented Aug 16, 2020

I said:

throws an error already :). I'll have to make this a nicer error message, is all!
sourmash.exceptions.Panic: sourmash panicked: thread 'unnamed' panicked with 'called Result::unwrap()on anErr value: InvalidHashFunction { function: "dna" }' at src/core/src/signature.rs:367

luiz said:

Yikes, a Panic... Oh, I see, this is the code where it is being thrown:

self.signatures 
    .iter_mut()  
    .for_each(|sketch| { 
        sketch.add_protein(&seq).unwrap(); }  
    );

I didn't know about try_for_each before, which allows errors, so the fix is

self.signatures 
    .iter_mut()  
    .try_for_each(|sketch| { 
        sketch.add_protein(&seq) }  
    )?;

This is going to raise a ValueError, which you can then capture and give a better error message


I made the changes in src/core/src/signature.rs, line 355 / method add_protein, and got:

   Compiling sourmash v0.9.0 (/Users/t/dev/sourmash/src/core)
error[E0277]: the trait bound `(): std::ops::Try` is not satisfied
   --> src/core/src/signature.rs:366:18
    |
366 |                 .try_for_each(|sketch| {
    |                  ^^^^^^^^^^^^ the trait `std::ops::Try` is not implemented for `()`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `sourmash`.

To learn more, run the command again with --verbose.
make: *** [build] Error 101

@ctb
Copy link
Contributor Author

ctb commented Aug 16, 2020

test code to trigger the panic is in #1172

@luizirber
Copy link
Member

I made the changes in src/core/src/signature.rs, line 355 / method add_protein, and got:

   Compiling sourmash v0.9.0 (/Users/t/dev/sourmash/src/core)
error[E0277]: the trait bound `(): std::ops::Try` is not satisfied
   --> src/core/src/signature.rs:366:18
    |
366 |                 .try_for_each(|sketch| {
    |                  ^^^^^^^^^^^^ the trait `std::ops::Try` is not implemented for `()`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `sourmash`.

To learn more, run the command again with --verbose.
make: *** [build] Error 101

Check to see if there is a ; at the end of that .try_for_each call. It should be

self.signatures 
    .iter_mut()  
    .try_for_each(|sketch| { 
        sketch.add_protein(&seq) }  
    )?;

not

self.signatures 
    .iter_mut()  
    .try_for_each(|sketch| { 
        sketch.add_protein(&seq); }  
    )?;

(putting the semicolon makes the |sketch| { sketch.add_protein(&seq); } closure return (), without the semicolon it is returning Result<(), Error> and then .try_for_each` will work)

@ctb
Copy link
Contributor Author

ctb commented Aug 17, 2020

thanks! that worked ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants