Skip to content

Fix: Allow toggling dynamic log links for Ray, Dask, Spark plugins#7193

Open
fg91 wants to merge 2 commits intomasterfrom
fg91/fix/ray-spark-dask-plugins-dynamic-log-links
Open

Fix: Allow toggling dynamic log links for Ray, Dask, Spark plugins#7193
fg91 wants to merge 2 commits intomasterfrom
fg91/fix/ray-spark-dask-plugins-dynamic-log-links

Conversation

@fg91
Copy link
Copy Markdown
Member

@fg91 fg91 commented Apr 10, 2026

Why are the changes needed?

Dynamic log links currently work for the pod plugin (normal PythonFunctionTask) and for the kubeflow plugin. Currently they cannot be toggled for Ray, Dask or Spark tasks. This PR fixes this.

What changes were proposed in this pull request?

The so-called TaskTemplate that contains the dynamic log link config needs to be included in the tasklog.Input{...} based on which the log links are generated.

How was this patch tested?

Added unit tests.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Signed-off-by: Fabio Grätz <fabio@cusp.ai>
@fg91 fg91 added the fixed For any bug fixes label Apr 10, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.96%. Comparing base (ed8aedb) to head (586f6ab).

Files with missing lines Patch % Lines
flyteplugins/go/tasks/plugins/k8s/dask/dask.go 50.00% 1 Missing and 1 partial ⚠️
flyteplugins/go/tasks/plugins/k8s/ray/ray.go 66.66% 1 Missing and 1 partial ⚠️
flyteplugins/go/tasks/plugins/k8s/spark/spark.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7193   +/-   ##
=======================================
  Coverage   56.96%   56.96%           
=======================================
  Files         931      931           
  Lines       58188    58200   +12     
=======================================
+ Hits        33146    33153    +7     
- Misses      22001    22003    +2     
- Partials     3041     3044    +3     
Flag Coverage Δ
unittests-datacatalog 53.51% <ø> (ø)
unittests-flyteadmin 53.14% <ø> (ø)
unittests-flytecopilot 43.06% <ø> (ø)
unittests-flytectl 64.07% <ø> (ø)
unittests-flyteidl 75.71% <ø> (ø)
unittests-flyteplugins 60.17% <62.50%> (+<0.01%) ⬆️
unittests-flytepropeller 53.65% <ø> (ø)
unittests-flytestdlib 63.00% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fg91 fg91 requested review from Sovietaced and pingsutw April 10, 2026 16:25
Copy link
Copy Markdown
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, Thank you

OccurredAt: &occurredAt,
}

taskTemplate, err := pluginContext.TaskReader().Read(ctx)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be an s3 hit if I recall.. do we want to do that every time?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay for now since we already do that for PythonFunctionTask. We can have a separate PR to do it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, here via this line 🤔

But this is a good point, I will check audit logs next week how many of those requests propeller makes!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good point...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fixed For any bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants