Proto field additions for JiT flow#2182
Conversation
rafal-hawrylak
requested review from
andrzej-grudzien
and removed request for
a team
rafal-hawrylak
force-pushed
the
pr4a-proto-fields
branch
2 times, most recently
from
085649a to
6c8b2c8
Compare
rafal-hawrylak
force-pushed
the
pr4a-proto-fields
branch
from
6c8b2c8 to
121879a
Compare
| // cancelled. Does not bound per-model JiT compilation; see jit_timeout_millis. | ||
| int32 timeout_millis = 7; | ||
|
|
||
| // Per-model JiT compilation worker timeout. Each action with JiT code gets |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // Current execution information for introspection. | ||
| RunningExecutionData execution_data = 7; | ||
| // File name where the target is defined. | ||
| string file_name = 8; |
There was a problem hiding this comment.
Why do we need it? Is it just for better error handling? (having it in PR description would be helpful)
There was a problem hiding this comment.
The file_name property is required to enable correct resolution of relative module imports (via require / import) inside the sandboxed VM during JiT compilation. Without it, the VM cannot determine the correct directory context of the SQLX/JS source code to resolve relative paths to dependencies in the project structure.
| string error_message = 2; | ||
| Timing timing = 3; | ||
| ExecutionMetadata metadata = 4; | ||
| string compiled_sql = 5; |
There was a problem hiding this comment.
Why do we need it here? (I have some guess, but please add a comment explaining when it is populated).
There was a problem hiding this comment.
compiled_sql is needed to expose the dynamically compiled SQL during dry-runs (--dry-run). Because JiT actions are compiled right before execution, their generated SQL is only available at runtime. Populating compiled_sql in TaskResult allows dry-run executions to output the final compiled SQL to the user. I will add a code comment explaining that this is populated during dry-run executions.
|
|
||
| string jit_code = 12; | ||
|
|
||
| bool disabled = 13; |
There was a problem hiding this comment.
Why do we need this now? I don't think it was required before and don't understand why we need it now.
There was a problem hiding this comment.
With the introduction of JiT, actions are compiled at execution time, meaning both enabled and disabled JiT actions initially have an empty tasks array. To prevent the runner from compiling and executing disabled JiT actions, the explicit disabled flag is required on the ExecutionAction proto so the runner can identify and skip them.
| // Per-model JiT compilation worker timeout. Each model with JiT code gets | ||
| // its own fresh budget; this is per-model, not a shared budget across the | ||
| // compile. | ||
| int32 jit_timeout_millis = 13; |
There was a problem hiding this comment.
Same as protos/execution.proto.
2 participants
Part of #2110 - [Integrate JiT compilation into CLI]