Skip to content

Proto field additions for JiT flow #2182

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

Merged
rafal-hawrylak merged 1 commit into from
Jul 2, 2026
Merged

Proto field additions for JiT flow #2182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion protos/core.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ message CompileConfig {
// Project configuration overrides.
ProjectConfig project_config_override = 3;

// Override compilation timeout settings.
// Override top-level compilation timeout (the single compile worker that
// runs once per invocation).
int32 timeout_millis = 6;

Extension extension = 10;
Expand Down
12 changes: 12 additions & 0 deletions protos/execution.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,16 @@ message RunConfig {
bool include_dependencies = 3;
bool include_dependents = 11;
bool full_refresh = 2;

// Wall-clock deadline for the entire run. When it fires, in-flight actions
// (including any running JiT compilation worker) are cancelled.
int32 timeout_millis = 7;

// Per-model JiT compilation worker timeout. Each action with JiT code gets

@ikholopov-omni ikholopov-omni Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we need it? Why not just use a generic timeout_millis? Scoping JiT stage of execution of timeout_millis looks like a broken contract to me.

@rafal-hawrylak rafal-hawrylak Jun 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

timeout_millis specifies the wall-clock deadline for the entire execution run (which can be hours). JiT compilation, however, is executed on a separate worker thread for each individual model immediately before its tasks run. If a single model's compilation hangs, allowing it to consume the entire timeout_millis would block the pipeline and starve other models. Each model needs its own small, independent compilation budget (defaulting to 60s) that resets per action.

@ikholopov-omni ikholopov-omni Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok. Why not only respect timeout_millis deadline and don't set per-jit task deadline at all - instead terminate them once an overarching timeout_millis is terminated?

@rafal-hawrylak rafal-hawrylak Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My idea here was to have separate budget for the workers so if among hundreds there is only one that hung or is long-running, all the rest would have a chance to be successfully executed. With global deadline this single action will eat the whole budget.

@ikholopov-omni ikholopov-omni Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think I communicated my idea well. Timeouts shouldn't be budgets. They are not additive. Timeout should be a deadline. A point in time after which the whole run should ramp up the activity. If a single action is hanging - ok, others are still running in their execution context. If the general timeout is reached, we terminate whoever failed to complete execution.

@rafal-hawrylak rafal-hawrylak Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok, a budget is not a fortunate word here - correct - timeouts should not add up.

But lets take another, bolder example. Lets say there is a separate timeout on connection to BQ or execution in BQ - which is a common practice. Such a timeout may coexist with the global timeout. Having dedicated timeouts usually allows more graceful custom subprocesses eviction.

@ikholopov-omni ikholopov-omni Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, so summing up, you will do the following:

  1. Have a global overarching timeout that covers the whole run. Once it is reached, whatever is running is terminated.
  2. In addition, it is possible to set JiT action timeout. If JiT action reaches it, it is marked as failed, but can free up an execution slot for other queued actions (if there is a concurrency limit hit).

Is this description full & correct now? If yes, please update comments and remove unnecessary confusing statements or terms (like "Does not bound per-model JiT compilation" or "budget").

@rafal-hawrylak rafal-hawrylak Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, udpated

// its own fresh deadline; independent of timeout_millis. When unset, no
// per-model cap is applied and only timeout_millis bounds JiT work.
int32 jit_timeout_millis = 10;

// For internal use only, will be removed at a later date.
bool disable_set_metadata = 9;

Expand Down Expand Up @@ -118,6 +126,10 @@ message TaskResult {
string error_message = 2;
Timing timing = 3;
ExecutionMetadata metadata = 4;
// SQL that would be executed. Populated only during --dry-run so that
// dry-run output can include the query — particularly for JiT actions
// whose SQL is only available after runtime compilation.
string compiled_sql = 5;
Comment thread
rafal-hawrylak marked this conversation as resolved.
}

message TestResult {
Expand Down
Loading