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

exit on null for target output #50

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

DE0CH
Copy link
Contributor

@DE0CH DE0CH commented Dec 19, 2022

This is mostly needed for auto-optimization/iracepy#22 because error from the user defined function would not propagate from a python function targetRunnerParallel to the embedded R instance. If I raise an error, rpy2 will just swallow the error and return NULL instead. I am not sure if this is a feature or a bug. But in any case, we need to stop irace at that point otherwise it will just go on and fail somewhere else, alerting the user of an "internal bug" and taking down the python interpreter with it due to auto-optimization/iracepy#27.

R/race-wrapper.R Outdated
@@ -518,6 +518,7 @@ execute.experiments <- function(experiments, scenario)
scenario$targetRunnerParallel(experiments, exec.target.runner,
scenario = scenario,
target.runner = target.runner)
if (is.null(target.output)) {stop()}
Copy link
Owner

Choose a reason for hiding this comment

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

This should give an irace.error() that mentions targetRunnerParallel.

Also, I'm considering that maybe we can get rid of the exec.target.runner parameter and use lapply() to call check_output_target_runner() after targetRunnerParallel returns. This means that the targetRunnerParallel does not benefit from automatic retries but you are anyway not using that feature and the other users of targetRunnerParalle also don't do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I'm considering that maybe we can get rid of the exec.target.runner parameter and use lapply() to call check_output_target_runner() after targetRunnerParallel returns. This means that the targetRunnerParallel does not benefit from automatic retries but you are anyway not using that feature and the other users of targetRunnerParalle also don't do that.

Yeah but would that require a major version bump because it would not be backward compatible (even if no one is using exec.target.runner)?

@DE0CH DE0CH force-pushed the exit-target-output branch from bd81fdd to 066cf59 Compare December 21, 2022 02:58
@DE0CH DE0CH requested a review from MLopez-Ibanez December 21, 2022 02:58
@MLopez-Ibanez MLopez-Ibanez merged commit 2df58b6 into MLopez-Ibanez:master Dec 21, 2022
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