Streaming Deck Squash #6162
Streaming Deck Squash #6162
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Code Review Agent Run #00f3fbActionable Suggestions - 2
Review Details
AI Code Review powered by |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6162 +/- ##
==========================================
- Coverage 37.02% 37.01% -0.01%
==========================================
Files 1317 1317
Lines 132534 132591 +57
==========================================
+ Hits 49067 49085 +18
- Misses 79221 79252 +31
- Partials 4246 4254 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Changelist by BitoThis pull request implements the following key changes.
|
| p.execInfo.OutputInfo = &handler.OutputInfo{ | ||
| OutputURI: outputPath, | ||
| } |
There was a problem hiding this comment.
Consider initializing OutputInfo with both OutputURI and DeckURI fields consistently. The current implementation only sets OutputURI which may lead to inconsistent state if DeckURI was previously set.
Code suggestion
Check the AI-generated fix before applying
| p.execInfo.OutputInfo = &handler.OutputInfo{ | |
| OutputURI: outputPath, | |
| } | |
| p.execInfo.OutputInfo = &handler.OutputInfo{ | |
| OutputURI: outputPath, | |
| DeckURI: nil, | |
| } |
Code Review Run #00f3fb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| return DeckUnknown, regErrors.Wrapf(err, "failed to read task template") | ||
| } | ||
|
|
||
| if template.GetMetadata().GetGeneratesDeck() { |
There was a problem hiding this comment.
Consider adding error handling for the case when GetMetadata() returns nil. Currently, if template.GetMetadata() returns nil, the code will panic when calling GetGeneratesDeck().
Code suggestion
Check the AI-generated fix before applying
| if template.GetMetadata().GetGeneratesDeck() { | |
| metadata := template.GetMetadata() | |
| if metadata == nil { | |
| return DeckUnknown, nil | |
| } | |
| if metadata.GetGeneratesDeck() { |
Code Review Run #00f3fb
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Tracking issue
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
Implementation of enhanced deck URI handling and real-time deck functionality in Flyte, including DeckUri support in node execution transformers and a new DeckStatus system. The changes improve deck URI handling during task execution while maintaining backward compatibility with older Flytekit versions. The implementation introduces robust deck file existence checks and enhanced deck generation capabilities.Unit tests added: False
Estimated effort to review (1-5, lower is better): 2