-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add update_execution func in remote #1860
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Da-Yi Wu <[email protected]>
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1860 +/- ##
==========================================
+ Coverage 55.01% 55.04% +0.02%
==========================================
Files 296 296
Lines 22216 22250 +34
Branches 2172 3358 +1186
==========================================
+ Hits 12223 12247 +24
+ Misses 9844 9840 -4
- Partials 149 163 +14
☔ View full report in Codecov by Sentry. |
Signed-off-by: Da-Yi Wu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a type hint for state? Not sure if there's a model object or something that should be used. Can also use a more user-friendly state in remote.py and only use the idl object in friendly.py.
Sure, in flyteidl the state is type of ExecutionState.
How about to use the int in remote.py and use ExecutionState in friendly.py |
Oh no enums are nice, let's just use it in both places. |
Use enum class make the usage not so straight-forward. User need to declare ExecutionState when update. But it is very clear.
On the other side, if we use typing.Literal. It is easy to use, but they need to look into the definition to see the meaning of value.
I use Literal first to make it simple. Comment on it if the first one is better or any other concern. |
Signed-off-by: Da-Yi Wu <[email protected]>
TL;DR
As Title
Type
Are all requirements met?