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

Close sequencefile if an error happens in open_reader #106

Merged
merged 2 commits into from
Nov 25, 2023

Conversation

luizirber
Copy link
Member

When running downstream projects with PYTHONDEVMODE=1 there is a ResourceWarning for unclosed files:

tests/test_cmd_signature.py::test_sig_kmers_1_dna_empty_seq                                                                                                                                    
  sourmash/.tox/py310/lib/python3.10/site-packages/screed/openscreed.py:35: ResourceWarning: unclosed file <_io.BufferedReader name='/tmp/nix-shell.7nFxtp/so
urmashtest_f12d2h09/query.fa'>                                                                                                                                                                 
    self.iter_fn = self.open_reader(filename, *args, **kwargs)                                                                                                                                 
                                                                                                                                                                                               
  Object allocated at:                                                                                                                                                                         
    File "sourmash/.tox/py310/lib/python3.10/site-packages/screed/openscreed.py", line 35                                                                    
      self.iter_fn = self.open_reader(filename, *args, **kwargs)   

Turns out we didn't close the sequencefile in the open_reader method of screed.openscreed.Open for two error cases before raising an exception, and with the fix in this PR the warnings disappear.

@luizirber luizirber requested a review from ctb November 24, 2023 20:01
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.57%. Comparing base (12bfb95) to head (aa64720).
Report is 3 commits behind head on latest.

Additional details and impacted files
@@            Coverage Diff             @@
##           latest     #106      +/-   ##
==========================================
+ Coverage   87.53%   87.57%   +0.03%     
==========================================
  Files          15       15              
  Lines         634      636       +2     
==========================================
+ Hits          555      557       +2     
  Misses         79       79              
Flag Coverage Δ
python 87.57% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@luizirber
Copy link
Member Author

Need #107 to fix RTD checks

@luizirber luizirber enabled auto-merge (squash) November 25, 2023 17:35
@luizirber luizirber merged commit 53f29eb into latest Nov 25, 2023
9 checks passed
@luizirber luizirber deleted the lirber/close_reader branch November 25, 2023 17:37
luizirber added a commit to sourmash-bio/sourmash that referenced this pull request Nov 25, 2023
Address
#974 (comment),
down from 281 warnings to 24 after
dib-lab/screed#106 is merged and a new `screed`
version is available.

Most warnings were `ResourceWarnings` for unclosed files in tests, but
there were a couple in `src/sourmash` too. Preferred to use a context
manager with `screed.open` or when passing the file to another library
(like `numpy`), but for tests changed a lot of code to use
`Path(...).read_text().splitlines()` instead.

The remaining 24 warnings are due to duplicate files in `ZipStorage`,
punting it for later because they will require more attention when
modifying `src/sourmash/sbt_storage.py`.
ctb added a commit to sourmash-bio/sourmash that referenced this pull request Mar 9, 2024
Fixes #2869

Screed release: https://github.com/dib-lab/screed/releases

The only really interesting/important update between 1.1.2 and 1.1.3 is
dib-lab/screed#106, which removes an unnecessary
warning when running tests.
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