fix: friendly logging #220
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a note for context (what errors are coming from wrapping the docker daemon, what are coming from actually running the container), and prints the stacktrace instead of just the error message.
|
Note that before this PR, errors were usually on one line like so (in reference to #78): Now, we have slightly formatted errors: Running DOMINO on framework "docker" with command: domino --active_genes_files /spras/QTS52HB/active_genes.txt --network_file /spras/KIFIAJG/network.txt --slices_file /spras/DHQFXEU/slices.txt --output_folder /spras/OGZE5QT/data1-domino-params-V3X4RW7 --use_cache false --parallelization 1 --visualization true --slice_threshold 0.3 --module_threshold 0.05
(Command formatted as list: `['domino', '--active_genes_files', '/spras/QTS52HB/active_genes.txt', '--network_file', '/spras/KIFIAJG/network.txt', '--slices_file', '/spras/DHQFXEU/slices.txt', '--output_folder', '/spras/OGZE5QT/data1-domino-params-V3X4RW7', '--use_cache', 'false', '--parallelization', '1', '--visualization', 'true', '--slice_threshold', '0.3', '--module_threshold', '0.05']`)
An unexpected non-zero exit status (1) inside the docker image docker.io/reedcompbio/domino occurred:
Traceback (most recent call last):
File "/usr/local/bin/domino", line 8, in <module>
sys.exit(main_domino())
File "/usr/local/lib/python3.7/site-packages/src/runner.py", line 36, in main_domino
G_final_modules=domino_main(active_genes_file=cur_ag, network_file=network_file, slices_file=slices_file, slice_threshold=slice_threshold, module_threshold=module_threshold)
File "/usr/local/lib/python3.7/site-packages/src/core/domino.py", line 349, in main
".")[0] + ".pkl"))
File "/usr/local/lib/python3.7/site-packages/src/core/domino.py", line 83, in prune_network_by_modularity
G_modularity = nx.algorithms.operators.union_all(G_modules)
File "/usr/local/lib/python3.7/site-packages/networkx/algorithms/operators/all.py", line 61, in union_all
raise ValueError('cannot apply union_all to an empty list')
ValueError: cannot apply union_all to an empty list |
Does this also make all the outputs (not just errors) from the docker containers look pretty? (It's fine if it doesn't) |
|
It does 👍 |
|
This broke all of the singularity test cases - I'll fix those now. |
stderr comes in buffer for docker, but not singularity? hm
|
Hm, it seems that these changes made some of the errors from Singularity that were usually just caught and printed bleed to the main python context - I'll have to install Singularity locally to properly fix these cases. |
formatted singularity errors!
|
Took a good while to get singularity installed, but that Also, the latest commit does pretty print singularity output as well! |
|
(CI failed but this time because of (now fixed) style issues! All tests pass 👍 ) |
tristan-f-r
commented
Jun 5, 2025
| except docker.errors.ContainerError as err: | ||
| print(f"(Command formatted as list: `{err.command}`)") | ||
| print(f"An unexpected non-zero exit status ({err.exit_status}) inside the docker image {err.image} occurred:") | ||
| err = str(err.stderr if err.stderr is not None else "", "utf-8") |
There was a problem hiding this comment.
The primary piece about this PR, as mentioned in slack, is that these err: str strings are constructed per container runtime, and can be easily handled. In another PR, we can have these errors be filtered for messages that we know are thrown by containers in certain situations, and we can handle them appropriately, while sufficiently raising exceptions of unknown errors to stop the Snakemake runtime and report those issues to SPRAS as a bug.
The current way that errors are handled have caused Aden and I a good amount of trouble when wrapping algorithms, since it's quite hard to tell that the output even is an error, and, if the algorithm that's being wrapped happens to touch the output file, it can sometimes make tests pass when they should fail. Even here, for example - if I were to raise this error instead of just printing it, it would break some tests, which is mainly why I wanted to break this into two separate PRs. 👍
Co-Authored-By: Justin Hiemstra <[email protected]>
agitter
reviewed
Jun 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds:
dockerd(without this, the error is some arbitrary 401 unauthorized error with no context, as the stacktrace was originally hidden.)Closes #51. In regards to that issue, it does not approach checking if the daemon has started 'optimally' (rather, it only tries to run docker and throws a friendly error if it can't run a container at all, nor does it try to do the docker detection at the start of the program.)