Skip to content

fix: Prevent binary log files to crash snakemake execution with show-failed-logs#2827

Merged
johanneskoester merged 2 commits intosnakemake:mainfrom
FroeseLab:fix/show-binary-logs
Apr 28, 2024
Merged

fix: Prevent binary log files to crash snakemake execution with show-failed-logs#2827
johanneskoester merged 2 commits intosnakemake:mainfrom
FroeseLab:fix/show-binary-logs

Conversation

@votti
Copy link
Copy Markdown
Contributor

@votti votti commented Apr 19, 2024

Description

As described in #2826 currently the whole snakemake execution crashes if a log file is binary and show-failed-logs is used.

This fix prevents the crash by capturing the UnicodeDecodeError and returning an error message that the log file is not a text instead.

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

@votti votti requested a review from johanneskoester as a code owner April 19, 2024 21:08
@votti votti changed the title Prevent binary log files to crash snakemake execution with show-failed-logs Fix: Prevent binary log files to crash snakemake execution with show-failed-logs Apr 19, 2024
@votti votti force-pushed the fix/show-binary-logs branch from 2deb30a to 2f25c1a Compare April 19, 2024 21:17
@votti votti changed the title Fix: Prevent binary log files to crash snakemake execution with show-failed-logs fix: Prevent binary log files to crash snakemake execution with show-failed-logs Apr 19, 2024
@votti votti force-pushed the fix/show-binary-logs branch from 2f25c1a to 4b7a9cc Compare April 19, 2024 21:23
Vito Zanotelli added 2 commits April 22, 2024 08:42
This replicates snakemake#2826

Unfortunately this test seems to currently crush the pytest test
execution when failing. Given that it should not fail any more
I think this may be acceptable.
Previously a binary log file would cause `show_logs` to crash the whole
snakemake workflow execution with a `UnicodeDecodeError`.

Closes snakemake#2826
@votti votti force-pushed the fix/show-binary-logs branch from 4b7a9cc to 7bddc8e Compare April 22, 2024 06:43
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@votti
Copy link
Copy Markdown
Contributor Author

votti commented Apr 22, 2024

@test: I am not extremely happy about the test: currently the test is more on the integration test level, so I think it may be better to also add a unittest for show_logs.
Currently this function is defined nested within the text_handler function, thus making it hard to test. To increase the testability , factoring it out as either a separate function in the logging module (eg _show_logs) or as a static method (Logger._show_logs).
Maybe it should be considered to factor out text_handler as a separate class containing all the methods defined withing the current text_handler function. This overall could be cleaner.

@johanneskoester
Copy link
Copy Markdown
Contributor

Sure, a follow-up PR in that direction would be appreciated. Maybe it would need synchronization with #2830

@johanneskoester johanneskoester merged commit 8a80bda into snakemake:main Apr 28, 2024
johanneskoester pushed a commit that referenced this pull request Apr 29, 2024
🤖 I have created a release *beep* *boop*
---


##
[8.11.1](v8.11.0...v8.11.1)
(2024-04-29)


### Bug Fixes

* check template rendering output for leaked input file paths
([#2850](#2850))
([433302e](433302e))
* do not distinguish between local and remote rules in dryrun
([74b99ec](74b99ec))
* omit norun jobs when determining remote storage input file retrieval
([#2854](#2854))
([37a7c7f](37a7c7f))
* Prevent binary log files to crash snakemake execution with
`show-failed-logs`
([#2827](#2827))
([8a80bda](8a80bda))
* replace pkg_resources for python 3.12
([#2831](#2831))
([ac144fc](ac144fc))
* return set instead of list when just --quiet
([#2829](#2829))
([eeb57e2](eeb57e2))
* small typo in error
([#2853](#2853))
([325a715](325a715))
* use keyword arguments in `_IOFile.open`
([#2847](#2847))
([50c84dc](50c84dc))


### Documentation

* fix typo and link for RO Crate
([#2851](#2851))
([cec0041](cec0041))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@votti votti deleted the fix/show-binary-logs branch June 13, 2024 08:42
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