feat: add custom job timeouts, also configurable per queue#2318
feat: add custom job timeouts, also configurable per queue#2318nhoening wants to merge 13 commits into
Conversation
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Documentation build overview
10 files changed ·
|
Keep the built-in forecasting queue timeout at one hour while the global default remains 180 seconds. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Log an error when job timeout configuration names a queue that FlexMeasures does not create. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Cover the preserved forecasting default, global default overrides, and unknown timeout queue logging. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Document the supported timeout queue names and the default forecasting queue timeout. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Mention that the new timeout settings preserve the one-hour forecasting default. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
BelhsanHmida
left a comment
BelhsanHmida
left a comment
There was a problem hiding this comment.
Looks good to me now.
I added some small follow-up commits to preserve backwards compatibility with forecasting and make the config safer:
- kept forecasting jobs on the existing 1-hour default timeout
- added an error log for unknown queue names like
"forecast" - covered the built-in defaults, global defaults, queue overrides, and typo handling in tests
- add small docs mention for forecasting default
I also manually tested this. Results matched expectations:
- built-in defaults: forecasting
3600s, scheduling/ingestion180s - custom global
PT5Mplus forecastingPT1H: forecasting3600s, scheduling/ingestion300s - typoed queue key
"forecast": logs an error and falls back to the global default
|
1 hour is a lot though. |
|
I don't think 1-hour forecasting jobs are defensible or needed in a live ems. Maybe in simulation, where they do a batch operation on years, but even that is parallelized by now, with afaik separate jobs either per vantage point (belief time) or per retraining frequency (so the same trained model gets reused for a few different vantage points with the job. In practice 5 minutes ought to be enough and should also be a practical limit, indeed to prevent clogging up the queue. And of course the whole point of this PR is that (e.g. simulation) users can easily customize the timeout now. |
|
My thinking around the 1-hour default was mostly based on simulation forecasting jobs, where jobs used to keep timing out. We solved much of that with parallelization per retraining cycle, so I agree that a 1-hour timeout is too generous as a default. You are both correct to point out that it can clog live EMS queues. So I think the regular default timeout is the better practical limit. For cases like simulations or slower hosts, users can now explicitly configure a longer timeout with One follow-up from that discussion is that timeout failures should be actionable: if a forecasting job times out, we should log clearly that it timed out and suggest either reducing the forecasting parameters/workload or increasing the timeout config. |
Let forecasting use the global RQ job timeout unless hosts configure a queue override. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Record and log an actionable hint when a forecasting job fails due to an RQ timeout. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Expect forecasting queues to inherit the global timeout by default. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Assert timeout failures log and persist a configuration hint on forecasting jobs. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Document the neutral per-queue timeout default and timeout guidance behavior. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <149331360+BelhsanHmida@users.noreply.github.com>
|
I pushed the follow-up to keep forecasting on the regular default timeout of 3 minutes, instead of preserving the old 1-hour timeout. I also added an actionable timeout message for forecasting jobs. If a forecasting job times out, we now log a hint and store it in the failed job metadata, suggesting splitting the forecast request, reducing |
| FORECASTING_JOB_TIMEOUT_HINT = ( | ||
| "Forecasting job timed out. Try splitting the forecast request into smaller " | ||
| "periods, reducing the prediction window by lowering `max-forecast-horizon`, " | ||
| "using fewer forecast cycles via `forecast-frequency`, or increasing " |
There was a problem hiding this comment.
Shouldn't the user use more forecast cycles by setting the forecast-frequency to smaller timedelta?
This comment is actually 3 notes in one:
- More cycles leads to more jobs, but it leads to shorter runtime per job. Fewer cycles would most likely increase the runtime, I think.
- Minor thing: the backticks around the field name are very unlikely to show up as a code snippet to the API user.
- A "lower frequency" is ambiguous. FlexMeasures follows Pandas terminology on this topic, which defines the frequency as a time unit, whereas it is actually one over time. That is, a low frequency normally would have a long duration between ticks. This is why I prefer explicitly mentioning what should happen to the timedelta instead.
| if isinstance(exc_type, type) and issubclass(exc_type, JobTimeoutException): | ||
| logger = logging.getLogger(__name__) | ||
| logger.warning(FORECASTING_JOB_TIMEOUT_HINT) | ||
| click.echo(FORECASTING_JOB_TIMEOUT_HINT) |
| "Forecasting job timed out. Try splitting the forecast request into smaller " | ||
| "periods, reducing the prediction window by lowering `max-forecast-horizon`, " | ||
| "using fewer forecast cycles via `forecast-frequency`, or increasing " | ||
| "FLEXMEASURES_JOB_TIMEOUT for the `forecasting` queue." |
There was a problem hiding this comment.
I would show this hint only in the log statement for hosts and not in the API user hint.
3 participants
Description
Hosts might want to increase the default timeout altogether (for slower hardware), or even per queue. This PR adds 2 config settings to do that.
Closes #2227
documentation/changelog.rstHow to test
Run flexmeasures/utils/job_utils.py
Notes
I found that there was a timeout setting for forecasts, in a rather obscure manner inside the code, giving forecasts 1 hour instead of 3 minutes.
So in that sense this PR is not completely backwards-compatible. @BelhsanHmida maybe you can comment on that.