Stamp 1-based FLYTE_ATTEMPT env var alongside FLYTE_ATTEMPT_NUMBER - #7792
Draft
pvditt wants to merge 1 commit into
Draft
Stamp 1-based FLYTE_ATTEMPT env var alongside FLYTE_ATTEMPT_NUMBER#7792pvditt wants to merge 1 commit into
pvditt wants to merge 1 commit into
Conversation
GetExecutionEnvVars now additionally stamps FLYTE_ATTEMPT, the 1-based attempt ordinal matching the wire/API convention (ActionAttemptIdentifier, console). FLYTE_ATTEMPT_NUMBER (0-based retry index) is unchanged and kept for existing SDK and user-code readers, so all combinations of old/new backend and old/new SDK keep working: new SDKs fall back to FLYTE_ATTEMPT_NUMBER + 1 when FLYTE_ATTEMPT is absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199AnC2fWkbhANzJtAKEV61 Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new 1-based attempt ordinal environment variable (FLYTE_ATTEMPT) to task pods so SDK/user code can read an attempt number consistent with the Flyte UI/wire conventions, while preserving the existing 0-based FLYTE_ATTEMPT_NUMBER for backward compatibility.
Changes:
- Stamp
FLYTE_ATTEMPT = RetryAttempt + 1alongside the existingFLYTE_ATTEMPT_NUMBER. - Update unit tests to account for the additional env var (counts/indices).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go | Adds the new FLYTE_ATTEMPT env var derived from RetryAttempt + 1. |
| flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds_test.go | Updates expectations for env var list size / placement in GetExecutionEnvVars tests. |
| flyteplugins/go/tasks/pluginmachinery/flytek8s/container_helper_test.go | Updates container env-var count expectation due to the added env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // (ActionAttemptIdentifier, console). FLYTE_ATTEMPT_NUMBER above is the | ||
| // 0-based retry index and is kept for existing SDK/user-code readers. | ||
| Name: "FLYTE_ATTEMPT", | ||
| Value: strconv.Itoa(int(id.GetID().RetryAttempt) + 1), |
Comment on lines
63
to
67
| envVars := GetExecutionEnvVars(mock, tt.consoleURL) | ||
| assert.Len(t, envVars, tt.expectedEnvVars) | ||
| if tt.expectedEnvVar != nil { | ||
| assert.Equal(t, tt.expectedEnvVar, &envVars[5]) | ||
| assert.Equal(t, tt.expectedEnvVar, &envVars[6]) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
Attempt numbering today is split across two conventions: the v2 wire/API (
ActionAttemptIdentifier, console, action events) is 1-based, while the execution layer'sTaskExecutionIdentifier.retry_attempt— and theFLYTE_ATTEMPT_NUMBERenv var derived from it — is a 0-based retry index inherited from v1. User code inside a task therefore sees0for the same attempt the console displays as "Attempt 1".This adds a 1-based
FLYTE_ATTEMPTenv var so SDKs can expose an attempt number consistent with the UI, without touching any existing consumer.What changes were proposed in this pull request?
GetExecutionEnvVarsstampsFLYTE_ATTEMPT(valueRetryAttempt + 1) alongside the unchangedFLYTE_ATTEMPT_NUMBER. Purely additive: old SDKs keep reading the old var; new SDKs readFLYTE_ATTEMPTand fall back toFLYTE_ATTEMPT_NUMBER + 1on backends without this change, so every old/new backend × old/new SDK combination behaves identically (companion SDK PR addsTaskContext.attemptwith that fallback).How was this patch tested?
Updated
TestGetExecutionEnvVarsandTestAddFlyteCustomizationsToContainerenv-var count/index expectations; suite passes.Labels
🤖 Generated with Claude Code
https://claude.ai/code/session_0199AnC2fWkbhANzJtAKEV61