Re-translate LaunchPlan fixed inputs with remote context on registration#3403
Open
Re-translate LaunchPlan fixed inputs with remote context on registration#3403
Conversation
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?
When a
LaunchPlanis created with aStructuredDatasetorFlyteFileas afixed_input, flytekit eagerly serializes those inputs to Flyte literals atLaunchPlan.create()time using the localFlyteContext. At that point, theFileAccessProviderpoints to a local/tmpdirectory, so any file or dataset upload goes there instead of remote storage. When the launch plan is later registered, the serialized literal contains a/tmppath that the remote Flyte cluster cannot access.What changes were proposed in this pull request?
LaunchPlan.__init__now stores_raw_fixed_inputs, the original Python native values passed asfixed_inputsbefore they are serialized to aLiteralMap.LaunchPlan.create()populates_raw_fixed_inputsalongside the existing_saved_inputs.FlyteRemote.register_launch_plan()now re-translatesraw_fixed_inputsusingself.context, which has aFileAccessProviderconfigured for remote storage. This overwrites the stale local literals before the launch plan is serialized and sent to Flyte Admin.How was this patch tested?
Added
test_register_launch_plan_retranslates_fixed_inputs_with_remote_contextintests/flytekit/unit/remote/test_remote.py. The test creates aLaunchPlanwith aFlyteFilefixed input and asserts thattranslate_inputs_to_literalsis called with the remote context (rr.context) duringregister_launch_plan. The test was also verified to fail against the pre-fix code, confirming it catches the regression.