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.

Sign up for GitHub

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

Jump to bottom

Don't sys.exit from run() function #2055

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Don't sys.exit from run() function #2055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
211a7d5
Don't sys.exit from run() function
ewels Sep 19, 2023
f7ad10c
Replace two other sys.exit() calls in multiqc.run()
ewels Sep 20, 2023
c1d7273
Merge branch 'master' into sys-exit
vladsavelyev Sep 20, 2023
c509ac8
Changelog
ewels Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ This idea goes way back to [issue #290](https://github.com/ewels/MultiQC/issues/
- Fixed parsing of `plot_type: "html"` `data` in json custom content
- Replace deprecated `pkg_resources`
- [Fix](https://github.com/ewels/MultiQC/issues/2032) the module groups configuration for modules where the namespace is passed explicitly to `general_stats_addcols`. Namespace is now always appended to the module name in the general stats ([2037](https://github.com/ewels/MultiQC/pull/2037)).
- Do not call `sys.exit()` in the `multiqc.run()` function, to avoid breaking interactive environments. [#2055](https://github.com/ewels/MultiQC/pull/2055)

### New Modules

Expand Down
6 changes: 3 additions & 3 deletions multiqc/multiqc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ def run(
run_modules = [m for m in run_modules if list(m.keys())[0] not in config.exclude_modules]
if len(run_modules) == 0:
logger.critical("No analysis modules specified!")
sys.exit(1)
return {"report": report, "config": config, "sys_exit_code": 1}
vladsavelyev marked this conversation as resolved.
Show resolved Hide resolved
run_module_names = [list(m.keys())[0] for m in run_modules]
logger.debug("Analysing modules: {}".format(", ".join(run_module_names)))

Expand Down Expand Up @@ -773,7 +773,7 @@ def __rich_measure__(self, console: rich.console.Console, options: rich.console.
shutil.rmtree(tmp_dir)
logger.info("MultiQC complete")
# Exit with an error code if a module broke
sys.exit(sys_exit_code)
return {"report": report, "config": config, "sys_exit_code": sys_exit_code}

if config.make_report:
# Sort the report module output if we have a config
Expand Down Expand Up @@ -981,7 +981,7 @@ def __rich_measure__(self, console: rich.console.Console, options: rich.console.
logger.error("Output directory {} already exists.".format(config.plots_dir))
logger.info("Use -f or --force to overwrite existing reports")
shutil.rmtree(tmp_dir)
sys.exit(1)
return {"report": report, "config": config, "sys_exit_code": 1}
Copy link
Member

@vladsavelyev vladsavelyev Sep 20, 2023 •
edited by ewels

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess because of these lines it also should never happen, unless a folder was created by another process.
https://github.com/ewels/MultiQC/blob/5f21841908b3771d9775a97521faf0a9737ad5d6/multiqc/multiqc.py#L933-L934

Simulated that, works fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you might be right there. I suspect that this code predates the rotating suffix thing being added. In which case, we can presumably just delete this chunk of code.

logger.info(
"Plots : {}{}".format(
os.path.relpath(config.plots_dir),
Expand Down