Skip to content

Don't drop shared inputs when switching task versions in the launch form - #7719

Merged
ursucarina merged 1 commit into
mainfrom
carina/converterfix
Jul 28, 2026
Merged

Don't drop shared inputs when switching task versions in the launch form#7719
ursucarina merged 1 commit into
mainfrom
carina/converterfix

Conversation

@ursucarina

Copy link
Copy Markdown
Contributor

Why are the changes needed?

When rerunning a run in the launch form and switching to a different task version, the form re-converts the original run's inputs against the selected version's interface via LiteralsToLaunchFormJson. That function returned an error the moment it hit a literal whose variable didn't exist in the target interface — which failed the entire conversion, so every input got dropped rather than just the one that no longer applies.

This produced an asymmetry when the two versions declare different inputs. For example, v3 has {name, count} and v17 has {name}:

  • v17 → v3: name is preserved and count is left empty to fill in — as expected.
  • v3 → v17: both clear, even though name exists in both versions.

The sibling conversion (JSONValuesToLiterals) already handles the analogous mismatch gracefully by ignoring unmapped fields with a warning. This change makes LiteralsToLaunchFormJson consistent: it skips literals that have no corresponding variable in the target interface (logging a warning) instead of erroring, so inputs shared between the two versions survive the switch in both directions.

What changes were proposed in this pull request?

LiteralsToLaunchFormJson no longer errors when a literal has no corresponding variable in the target VariableMap. Instead of returning CodeInvalidArgument and failing the whole conversion, it now skips that literal (logging a warning) and continues, so the literals that do map are still converted. This mirrors the existing unmapped-field handling in JSONValuesToLiterals.

Added a regression test that converts {name, count} against a {name}-only interface and asserts name is preserved with its value while count is dropped.

How was this patch tested?

Unit test in runs/service/converter/literal_json_converter_test.go — a new TestLiteralsToJsonSchema subtest converts a run's {name, count} literals against a target interface that only declares {name}, and asserts the call succeeds, name is preserved with its value, and the unmapped count is dropped. Ran go test ./runs/service/converter/..., all passing.

Also verified manually in the launch form: rerunning a run and switching from a higher-input version to a lower-input version now keeps the values for the shared inputs instead of clearing everything.

Labels

Please add one or more of the following labels to categorize your PR:

  • added: For new features.
  • changed: For changes in existing functionality.
  • deprecated: For soon-to-be-removed features.
  • removed: For features being removed.
  • fixed: For any bug fixed.
  • security: In case of vulnerabilities

This is important to improve the readability of release notes.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Stack

If you do use git town to manage PR Stacks, the stack relevant to this PR
will show below. Otherwise, you can ignore this section.

Docs link

@ursucarina ursucarina added the changed For changes in existing functionality label Jul 28, 2026
Copilot AI review requested due to automatic review settings July 28, 2026 17:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@ursucarina
ursucarina changed the base branch from main to v2 July 28, 2026 17:07
@ursucarina ursucarina changed the title Carina/converterfix Don't drop shared inputs when switching task versions in the launch form Jul 28, 2026
EngHabu
EngHabu previously approved these changes Jul 28, 2026
@ursucarina
ursucarina enabled auto-merge (squash) July 28, 2026 17:12
@ursucarina
ursucarina disabled auto-merge July 28, 2026 18:19
@ursucarina
ursucarina changed the base branch from v2 to main July 28, 2026 18:21
@ursucarina
ursucarina dismissed EngHabu’s stale review July 28, 2026 18:21

The base branch was changed.

Copilot AI review requested due to automatic review settings July 28, 2026 18:22
@ursucarina
ursucarina force-pushed the carina/converterfix branch from ad5b7d1 to dbdb1ba Compare July 28, 2026 18:22
@ursucarina
ursucarina enabled auto-merge (squash) July 28, 2026 18:23
@ursucarina
ursucarina merged commit 6adca19 into main Jul 28, 2026
25 checks passed
@ursucarina
ursucarina deleted the carina/converterfix branch July 28, 2026 18:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

dataproxy/converter/literal_json_converter.go:399

  • logger.Warnf is emitted once per skipped literal. In cases where the target interface drops many fields (or has no inputs at all), this can generate a large number of warnings for a single conversion and add noise/cost in logs. Consider collecting skipped literal names during the loop and logging a single warning after the loop (similar to the aggregated unmapped-field warning in JSONValuesToLiterals).
			// inputs the two versions *share* are still preserved. Mirrors the
			// unmapped-field handling in JSONValuesToLiterals.
			logger.Warnf(ctx, "skipping literal %q with no corresponding variable in the target interface (ignoring)", literal.GetName())
			continue
		}

// them (with a warning) instead of failing the whole conversion, so the
// inputs the two versions *share* are still preserved. Mirrors the
// unmapped-field handling in JSONValuesToLiterals.
logger.Warnf(ctx, "skipping literal %q with no corresponding variable in the target interface (ignoring)", literal.GetName())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we make this debug?

@wild-endeavor wild-endeavor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lower log but +1

@ursucarina ursucarina mentioned this pull request Jul 28, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changed For changes in existing functionality flyte2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants