Add InstanceInfo to ActionEvent and ActionAttempt protos - #7803
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the run-definition API surface to capture and propagate per-attempt cloud node/instance metadata (e.g., instance type, zone, node name) so action events/attempts can attribute runtime to the underlying compute instance.
Changes:
- Adds a new
InstanceInfoproto message and wires it intoActionEventandActionAttempt. - Regenerates Go protobuf + validation bindings to include the new message/fields.
- Extends pluginmachinery
TaskInfowith aNodeNamefield to carry scheduled node name in single-pod execution paths.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| flyteidl2/workflow/run_definition.proto | Adds InstanceInfo message and new instance_info fields on event/attempt messages. |
| gen/go/flyteidl2/workflow/run_definition.pb.go | Regenerated Go protobuf bindings reflecting InstanceInfo + new fields. |
| gen/go/flyteidl2/workflow/run_definition.pb.validate.go | Regenerated validation code including InstanceInfo validation stubs and embedded validation calls. |
| flyteplugins/go/tasks/pluginmachinery/core/phase.go | Adds TaskInfo.NodeName for passing scheduled node name through pluginmachinery. |
Files not reviewed (2)
- gen/go/flyteidl2/workflow/run_definition.pb.go: Generated file
- gen/go/flyteidl2/workflow/run_definition.pb.validate.go: Generated file
Suppressed comments (1)
flyteplugins/go/tasks/pluginmachinery/core/phase.go:126
TaskInfo.String()dereferencest.OccurredAtwithout a nil check, which can panic if aTaskInfo{}is ever formatted/logged (and there are multiple call sites that constructTaskInfo{}without settingOccurredAt). Consider guarding against nil to keepString()safe.
NodeName string
}
func (t *TaskInfo) String() string {
return fmt.Sprintf("Info<@%s>", t.OccurredAt.String())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 8 changed files in this pull request and generated no new comments.
Files not reviewed (3)
- gen/go/flyteidl2/workflow/run_definition.pb.go: Generated file
- gen/go/flyteidl2/workflow/run_definition.pb.validate.go: Generated file
- gen/python/flyteidl2/workflow/run_definition_pb2.py: Generated file
Suppressed comments (2)
flyteplugins/go/tasks/pluginmachinery/core/phase.go:122
- Adding a new exported field to TaskInfo is a source-breaking API change for any downstream code using positional composite literals (e.g.
TaskInfo{a, b, ...}), which will no longer compile. This is inherent to Go structs; consider documenting that TaskInfo must be constructed with keyed fields so future additions don't unexpectedly break consumers.
// Name of the node the task's (primary) pod was scheduled on, when known.
// Only populated by single-pod execution paths; empty for multi-pod plugins.
NodeName string
flyteidl2/workflow/run_definition.proto:398
- The comment says capacity_type is normalized to "spot" or "on-demand", but the schema allows any string and has no validation. To keep producers/consumers interoperable, consider modeling this as an enum (preferred) or adding a buf.validate constraint (e.g. string.in) so unexpected values are rejected early.
// Capacity type of the node, normalized to "spot" or "on-demand".
string capacity_type = 4;
Adds an InstanceInfo message (instance type, instance id, accelerator, capacity type, zone, node name, attributes escape hatch) describing the node an action-attempt's pod was scheduled on: - ActionEvent.instance_info = 17: attached by the executor when known - ActionAttempt.instance_info = 15: read surface, merged from events by the run service (merge semantics: latest event carrying instance info fixes the attempt's node; same-node older events may upgrade a node-name-only record to a resolved one) - TaskInfo.NodeName: populated by single-pod execution paths so an executor can resolve node metadata off the hot path Only the proto/plugin-core surface is added here; the flyte2 runs service does not populate or merge the field yet. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PMNn4hYoYsVZWdUB4Cs9iJ Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
ef0b557 to
8c21e8e
Compare
wild-endeavor
left a comment
There was a problem hiding this comment.
add region and provider which is an enum
| // Accelerator device attached to the node, from the cloud accelerator label | ||
| // (e.g. "cloud.google.com/gke-accelerator" on GCP). Typically empty on AWS, | ||
| // where the instance type alone identifies the GPU. | ||
| string accelerator = 3; |
There was a problem hiding this comment.
Why are the changes needed?
We want action events to record which cloud instance (node) an action-attempt's pod actually ran on — e.g.
p4d.24xlarge— so it can be shown per attempt and used for GPU/cost attribution. Today nothing captures this at runtime.What changes were proposed in this pull request?
Proto/plugin-core surface only (no executor or run-service behavior change in this repo):
InstanceInfomessage: instance type, instance id, accelerator, capacity type (spot/on-demand), zone, node name, plus an attributes map for future use.ActionEvent.instance_info = 17— set by an executor when it knows the node.ActionAttempt.instance_info = 15— read-side surface, filled by whatever run service merges events.TaskInfo.NodeNamein pluginmachinery — lets single-pod execution paths pass the scheduled node name along, so an executor can resolve node metadata off the hot path.The flyte2 executor/run service does not populate or merge the field yet — consumers of these protos (Union leaseworker) do.
How was this patch tested?
buf lintpasses; Go bindings regenerated with the pinned buf templates.go build+ existing test suites forruns/serviceandflytepluginspass.GetActionDetails.Labels
added
🤖 Generated with Claude Code
https://claude.ai/code/session_01PMNn4hYoYsVZWdUB4Cs9iJ