Update pyflyte defaults to use --copy behavior #2755
Conversation
Signed-off-by: Yee Hing Tong <[email protected]>
wild-endeavor
requested review from
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
wild-endeavor
changed the title
Signed-off-by: Yee Hing Tong <[email protected]>
| " Note this needs additional configuration, refer to the docs.", | ||
| ) | ||
| @click.option( | ||
| "--copy", | ||
| required=False, | ||
| type=click.Choice(["all", "auto", "none"], case_sensitive=False), | ||
| default=None, # this will be changed to "none" after removing fast option | ||
| default="none", |
There was a problem hiding this comment.
As long as --fast is in the codebase, someone can still run --fast --copy auto, which is conflicting/confusing. I like keeping the default for copy as None and raising the current error.
When we actually remove --fast as an option, then I feel safe enough to set default="none".
There was a problem hiding this comment.
we won't remove it though for a while, at least a year or two... so just keep it around?
There was a problem hiding this comment.
can we detect (and fail) in this case? Also, add a message that explains how to fix the issue.
There was a problem hiding this comment.
also did you mean this for package specifically or the commands in general? if so we can just discard this PR... but that leaves the old logic and doesn't exercise the new logic unless it's opted into.
There was a problem hiding this comment.
we can detect and fail yeah... but i'd like to exercise the new code
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Why are the changes needed?
This is the last of a series of PRs to clean up the fast register switches in the various
pyflytecommands. The main change was #2690This PR changes the default behavior for fast registration (invoked by
run,register, andpackageto use the new logic instead. Previously if the--copycommand was not specified, the old logic was still used. Only theserializecommand after this PR will use the old logic.What changes were proposed in this pull request?
How was this patch tested?
Locally tested and local executions run.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
#2690
#2716
#2731
Docs link