-
Notifications
You must be signed in to change notification settings - Fork 304
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
[flyte deck] Streaming Decks #2779
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2779 +/- ##
===========================================
+ Coverage 51.08% 82.79% +31.71%
===========================================
Files 201 3 -198
Lines 21231 186 -21045
Branches 2731 0 -2731
===========================================
- Hits 10846 154 -10692
+ Misses 9787 32 -9755
+ Partials 598 0 -598 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
I would love to see an example of tqdm |
replaces #1704 |
will try it, just let me ship the MSGPACK IDL first. |
flytekit/deck/deck.py
Outdated
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> |
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.
This will make it automatically refresh the page.. probably need that on the empty deck but not sure how to do the realtime behavior once it has content (you don't want to refresh while the user is browsing around or clicking through various tabs)
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | |
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | |
<meta http-equiv="refresh" content="5" > |
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.
COOL, will try it, thank you
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #24744dActionable Suggestions - 2
Additional Suggestions - 1
Review Details
|
Signed-off-by: Future-Outlier <[email protected]>
Changelist by BitoThis pull request implements the following key changes.
|
flytekit/deck/deck.py
Outdated
# todo: change to a more proper error | ||
raise ValueError("Deck is disabled for this task, please don't call Deck.publish()") |
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.
Consider using a more specific exception type like DeckDisabledException
instead of ValueError
to better communicate the error condition. The TODO comment also suggests this needs improvement.
Code suggestion
Check the AI-generated fix before applying
# todo: change to a more proper error | |
raise ValueError("Deck is disabled for this task, please don't call Deck.publish()") | |
class DeckDisabledException(Exception): | |
pass | |
raise DeckDisabledException("Deck is disabled for this task, please don't call Deck.publish()") |
Code Review Run #24744d
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flytekit/core/base_task.py
Outdated
if not self.disable_deck: | ||
ctx.user_space_params._enable_deck = True # type: ignore | ||
if DeckField.TIMELINE.value in self.deck_fields and ctx.user_space_params is not None and not self.disable_deck: |
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.
Consider combining the two conditional checks into a single condition to avoid redundant disable_deck
checks. The current implementation checks disable_deck
twice which introduces redundancy.
Code suggestion
Check the AI-generated fix before applying
if not self.disable_deck: | |
ctx.user_space_params._enable_deck = True # type: ignore | |
if DeckField.TIMELINE.value in self.deck_fields and ctx.user_space_params is not None and not self.disable_deck: | |
if not self.disable_deck and ctx.user_space_params is not None: | |
ctx.user_space_params._enable_deck = True | |
if DeckField.TIMELINE.value in self.deck_fields: |
Code Review Run #24744d
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
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.
yeah we can do this, small change.
Signed-off-by: Future-Outlier <[email protected]>
flyteconsole will send a request to flyteadmin, and flyteadmin will give it the deck html |
Code Review Agent Run #75fbe2Actionable Suggestions - 0Review Details
|
if isinstance(entity.task_function, ClassDecorator): | ||
extra_config = entity.task_function.get_extra_config() | ||
if not entity.disable_deck: | ||
entity.metadata.generates_deck = True |
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 this to a unit test?
flytekit/deck/deck.py
Outdated
_output_deck(task_name=task_name, new_user_params=params) | ||
else: | ||
# todo: change to a more proper error | ||
raise ValueError("Deck is disabled for this task, please don't call Deck.publish()") |
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.
Should we just log a warning instead of raising an error? the warning can say that the call to publish is being ignored. that way the task can continue to function.
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.
Also let's remove the warning on line 46: "This feature is in beta."
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.
ok I'm going to call warning
flytekit/core/base_task.py
Outdated
if not self.disable_deck: | ||
ctx.user_space_params._enable_deck = True # type: ignore | ||
if DeckField.TIMELINE.value in self.deck_fields and ctx.user_space_params is not None and not self.disable_deck: |
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.
yeah we can do this, small change.
flytekit/core/base_task.py
Outdated
@@ -720,7 +723,9 @@ def dispatch_execute( | |||
may be none | |||
* ``DynamicJobSpec`` is returned when a dynamic workflow is executed | |||
""" | |||
if DeckField.TIMELINE.value in self.deck_fields and ctx.user_space_params is not None: | |||
if not self.disable_deck: | |||
ctx.user_space_params._enable_deck = True # type: ignore |
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.
since the ExecutionParams
object has a builder, maybe we should use it here instead of accessing a private field?
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.
ok I'll move it to the builder, thank you
flytekit/deck/deck.py
Outdated
@@ -86,6 +86,16 @@ def name(self) -> str: | |||
def html(self) -> str: | |||
return self._html | |||
|
|||
@classmethod |
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.
technically this can be static right?
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.
yes static is better I think
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.
since static method is bound to a class rather than the objects for that class.
reference: https://www.digitalocean.com/community/tutorials/python-static-method
flytekit/deck/deck.py
Outdated
@@ -86,6 +86,16 @@ def name(self) -> str: | |||
def html(self) -> str: | |||
return self._html | |||
|
|||
@classmethod | |||
def publish(cls): |
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.
Deck.publish, pulls the ExecutionParams
from the current flytekit context, and calls _output_deck
, which again calls the current context to get at the file_access code, to do an upload. what do you think about moving this error check inside _output_deck? making this function just a wrapper around that?
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.
yes we can do this, this can make the code more efficient
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.
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.
now it changed to warning, and move the logic to _output_deck
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
…to _output_deck Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #6220fdActionable Suggestions - 2
Review Details
|
task_name = params.task_id.name | ||
_output_deck(task_name=task_name, new_user_params=params) |
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.
Consider checking if params.task_id
is not None
before accessing the name
attribute to avoid potential NoneType
errors.
Code suggestion
Check the AI-generated fix before applying
@@ -87,3 +87,6 @@ def publish():
params = FlyteContextManager.current_context().user_space_params
- task_name = params.task_id.name
- _output_deck(task_name=task_name, new_user_params=params)
+ if params.task_id is None:
+ raise ValueError("Task ID is not available in the current context")
+ task_name = params.task_id.name
+ _output_deck(task_name=task_name, new_user_params=params)
Code Review Run #6220fd
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flytekit/deck/deck.py
Outdated
if not params.enable_deck: | ||
logger.warning("Deck is disabled for this task, please don't call Deck.publish()") |
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.
Consider checking params
for None
before accessing enable_deck
attribute to avoid potential NoneType
attribute errors.
Code suggestion
Check the AI-generated fix before applying
if not params.enable_deck: | |
logger.warning("Deck is disabled for this task, please don't call Deck.publish()") | |
if params is None or not params.enable_deck: | |
logger.warning("Deck is disabled for this task or context params are not available, please don't call Deck.publish()") | |
return |
Code Review Run #6220fd
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #3bf552Actionable Suggestions - 0Review Details
|
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #b6d959Actionable Suggestions - 0Additional Suggestions - 1
Review Details
|
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #f6709fActionable Suggestions - 0Review Details
|
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #7a0ea5Actionable Suggestions - 0Review Details
|
Tracking issue
flyteorg/flyte#5574
Why are the changes needed?
we want to support streaming decks.
What changes were proposed in this pull request?
add a function
publish
for the Flyte Deck.How was this patch tested?
Setup process
single binary.
flyte: flyteorg/flyte#6053
flytekit: #2779
flyteconsole: flyteorg/flyteconsole#890
Screenshots
flytekit: this branch
NEW FLYTEKIT, NO DECK, RUNNING With Deck, SUCCEED, and FAILED
OSS-STREAMING-DECK-small.mov
OLD FLYTEKIT, NO DECK, RUNNING With Deck, SUCCEED, and FAILED
OSS-STREAMING-DECK-OLD-FLYTEKIT-small.mov
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
Implementation of streaming deck functionality in Flytekit, featuring Deck.publish() method and infrastructure updates. Enhanced deck generation in base_task.py with proper protobuf boolean wrappers and parameter documentation. Refactored deck enabling mechanism by removing legacy flags and implementing builder pattern. Updated from positive to negative logic check and aligned test assertions with new implementation.Unit tests added: True
Estimated effort to review (1-5, lower is better): 2