Add support for specifying execution cluster labels in pyflyte #2422
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
va6996
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy and
samhita-alla
as code owners
|
Thanks for making the PR. I have not looked at it in detail but I think we need to upgrade the required version of Line 23 in 76fb7c3 Older versions seem to be missing the |
There was a problem hiding this comment.
I was halfway through implementing the exact same thing because we need this as well when I figured I should check whether somebody else is working on this given that the backend recently added support for this.
So thank you for having added this functionality to flytekit 🙏
I tested your PR in a GCP-based multi-cluster deployment and can confirm that the cluster selection works. (For the record: I didn't use pyflyte but FlyteRemote directly.)
I proposed a minor rewording suggestion for the doc strings, not blocking though, so LGTM after addressing @Tom-Newton 's comment about the flyteidl version.
| @@ -239,7 +239,7 @@ class RunLevelParams(PyFlyteParams): | |||
| ) | |||
| limit: int = make_click_option_field( | |||
| click.Option( | |||
| param_decls=["--limit", "limit"], | |||
| param_decls=["--limit"], | |||
There was a problem hiding this comment.
Nit: bit of scope creep ;)
Still valid of course.
va6996
force-pushed
the
ecl
branch
2 times, most recently
from
516c374
to
ccfd564
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2422 +/- ##
==========================================
+ Coverage 71.81% 76.04% +4.22%
==========================================
Files 183 182 -1
Lines 18553 18565 +12
Branches 3652 3654 +2
==========================================
+ Hits 13324 14117 +793
+ Misses 4587 3821 -766
+ Partials 642 627 -15 ☔ View full report in Codecov by Sentry. |
| @@ -256,6 +256,16 @@ class RunLevelParams(PyFlyteParams): | |||
| help="Assign newly created execution to a given cluster pool", | |||
| ) | |||
| ) | |||
| execution_cluster_label: str = make_click_option_field( | |||
| click.Option( | |||
| param_decls=["--execution-cluster-label", "-ecl"], | |||
There was a problem hiding this comment.
| param_decls=["--execution-cluster-label", "-ecl"], | |
| param_decls=["--execution-cluster-label", "--ecl"], |
Should there be two hyphens? See e.g. ["--envvars", "--env"].
There was a problem hiding this comment.
My intent was to use one similar to how you'd have for --version = -v? Lmk what you think. -ecl should work too.
There was a problem hiding this comment.
Looking at the existing args ...
param_decls=["--envvars", "--env"],
param_decls=["--raw-output-data-prefix", "--raw-data-prefix"],
param_decls=["-r", "--remote"],... I'd say currently we only use a single hyphen if it is followed by a single character like -v or -r. If multiple characters follow like --env, we use two hyphens.
I don't have strong opinions about this but I'd propose to just stick with -- here as well.
|
@eapolinario any idea of how to fix the CI check? |
|
@davidmirror-ops , CI failure is being fixed in #2477 |
@va6996 the fix has been merged. Could you please rebase? After that, the PR can be approved from my side. |
Signed-off-by: va6996 <[email protected]>
|
@eapolinario @cosmicBboy @wild-endeavor @pingsutw if any of you can review, we can unblock this PR. Thanks! |
|
Congrats on merging your first pull request! 🎉 |
Tracking issue
NA
Why are the changes needed?
Today we support the execution cluster label for specifying the execution cluster on an execution level. However there is no way to do that using the python sdk/pyflyte. This change adds support for the above mentioned.
What changes were proposed in this pull request?
We add another flag to pyflyte to allow users to specify the cluster label. Additionally when launching a wf, this info is passed to flyteadmin via the sdk.
How was this patch tested?
This patch was tested on a local instance of flyte. Test cases were modified to validate the changes.
Setup process
pyflyte run -ecl my-label mywf.py wfScreenshots
Check all the applicable boxes
Related PRs
Docs link