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

Error when frameworks installation script fails #685

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PGijsbers
Copy link
Collaborator

If we detect an error during setup, we want to stop the benchmark process immediately, instead of continuing.
Currently, the failed process goes unnoticed and often results in unexpected errors like missing dependencies during the execution of the exec.py script.

amlb/utils/process.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@eddiebergman eddiebergman left a comment

Choose a reason for hiding this comment

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

It seems it would be easier to detect this at the level of the subprocess call, rather than injecting a -e, this in effect offloads to the responsibility to the creator of the script to exit with the correct status code.

This would require a template that includes and documents a -e call in there, so people are aware of it if not super familiar with writing bash scripts. People writing a script for Windows, of which I imagine there are not too many, would have to do so in the way expected of a .bat file or whatever it is.

@PGijsbers
Copy link
Collaborator Author

Thanks for your thoughts!

detect this at the level of the subprocess call

Sorry, I am not 100% sure I understand. Are you suggesting that the scripts themselves invoke set -e or similar? Meaning effectively no change, other than perhaps documenting and encouraging the use of set -e for setup scripts? E.g., include it in a template such as https://github.com/PGijsbers/amlb-template/blob/main/interpretml/setup.sh.
Since the integration scripts are user code (from the benchmark perspective), I'd generally prefer a more defensive approach. Though I could live with simply embedding it in current scripts + a template, if setting it through the automl benchmark is a bad idea.

Windows isn't officially supported, doesn't work with any current setup scripts, and we currently have no plan to support it (outside of docker mode), so I would prefer to find the best solution for unix-based machines.

@eddiebergman
Copy link
Collaborator

It's not too much of an issue in the end, just need to ensure that using set -e twice isn't an issue.

I'm usually for offloading these issues to the user, namely just because it's hidden behaviour and might end up with their own manual running being different than what the benchmark does. For example, the git clone <src> <dest> will error if <dest> exists, but this is not always an issue for the setup, as you might still want to checkout a certain version.

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