Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat: Allow controlling in which task phases log links are shown #4726
Feat: Allow controlling in which task phases log links are shown #4726
Changes from 34 commits
0ba3b4a
02d1067
666b209
9324f10
52c1a61
de4de1a
ab93bc5
703548a
1873c3d
8122e76
2c742a5
039d922
c73e59f
0bb5fc2
79d51f5
0c8bdc3
d4102ac
66db326
720f053
24c8a80
382ed0a
105b766
383c043
0aa3d81
459c70d
e494723
5dc8525
edf30fc
2e54f69
dfc44b4
f8c68c5
7bfc8bc
683a6a5
f557e37
3bfba55
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
On a high level, the contract between flyteplugins/flytepropeller and flyteadmin changes the following way:
Before:
Running
phase, only exception being the spark plugin), the plugin needs to send this log link in that phase.After:
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.
I did briefly consider whether a plugin should be able to send a log link which will e.g. be shown only from
Running
onwards only a single time already in a prior phase, e.g.Pending
. This would mean that flyteadmin would have to track the still inactive log link and then make it active once the running phase is reached.I personally think this is an overkill though since currently all plugins always send all links. I feel the contract "If you want a log link to be shown from a specific phase onwards, you need to send the log link at least in this phase" is reasonable.
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 is required for map tasks which treat log links differently for performance/scalability reasons.
Master:
This branch:
Note that the task execution view with the log links only becomes available in the running phase for map tasks, meaning that there is no use case for showing log links already in the pending phase. However, log links can be removed once finished.
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.
Note: array node map tasks replaced map tasks in flytekit 1.12.0 and this distinction is no longer true, i.e. array node map tasks log links no longer use the separate map tasks log links.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.