Add GetDeckPath to OutputReader

[pingsutw]

Signed-off-by: Kevin Su pingsutw@apache.org

TL;DR

In order to cache the deck file, we need to add a new attribute DeckPath to InMemoryOutputReader

Related flyteorg/flytepropeller#443

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#2175

Follow-up issue

NA

[codecov]

Codecov Report

Merging #268 (a5d0aa0) into master (2c83e24) will increase coverage by 0.45%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
+ Coverage   62.55%   63.01%   +0.45%     
==========================================
  Files         142      142              
  Lines        8960     8968       +8     
==========================================
+ Hits         5605     5651      +46     
+ Misses       2834     2796      -38     
  Partials      521      521              

Flag	Coverage Δ
unittests	62.36% <60.00%> (+0.32%)	⬆️

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

Impacted Files	Coverage Δ
go/tasks/pluginmachinery/ioutils/paths.go	84.61% <ø> (ø)
go/tasks/plugins/array/awsbatch/monitor.go	61.79% <0.00%> (ø)
go/tasks/plugins/array/k8s/management.go	55.77% <0.00%> (ø)
go/tasks/plugins/k8s/sagemaker/builtin_training.go	72.00% <0.00%> (ø)
...sks/plugins/k8s/sagemaker/hyperparameter_tuning.go	60.45% <0.00%> (ø)
go/tasks/plugins/presto/execution_state.go	50.65% <0.00%> (ø)
go/tasks/plugins/array/outputs.go	73.29% <50.00%> (ø)
...pluginmachinery/ioutils/in_memory_output_reader.go	78.94% <100.00%> (+78.94%)	⬆️
...uginmachinery/ioutils/remote_file_output_reader.go	20.54% <100.00%> (+3.40%)	⬆️
...uginmachinery/ioutils/remote_file_output_writer.go	58.33% <100.00%> (+58.33%)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c83e24...a5d0aa0. Read the comment docs.

[EngHabu]

We might want to think about this (maybe open a separate issue) for how to aggregate (if any) these html files in array case

[pingsutw]

okay, will create a new issue for it.

[EngHabu]

Looks like you may need to add a few more unit tests for patch coverage...

[honnix]

This is unfortunately a breaking change that we could have been more verbose and bumped minor (even major) version. One of our plugins needs to be updated to incorporate this. What is deck? Can we pass a nil if our plugin doesn't care?

[honnix]

What is deck? Can we pass a nil if our plugin doesn't care?

OK I found it here https://docs.flyte.org/projects/cookbook/en/latest/auto/core/flyte_basics/deck.html#sphx-glr-auto-core-flyte-basics-deck-py. Seem to be quite useful. So how we do pass in the path instead of nil?

[pingsutw]

@honnix what error did you run into? you can set deckPath to nil in NewInMemoryOutputReader.
In flytekit, output.pb will be written to output_dir/output.pband Flyte deck (an HTML file) will be written to output_dir/deck.html

https://github.com/pingsutw/flytepropeller/blob/f08ccaad75246c7a1149be632206c0278608e61e/pkg/controller/nodes/executor.go#L239-L249

Some plugins (like bigquery) that will not generate a deck file, we also set deckPath to nil.

[honnix]

Thanks for replying @pingsutw .

what error did you run into?

Since the signature of NewInMemoryOutputReader was changed by adding one more argument, one of our internal plugins failed to compile. Since this type of APIs could be used by any plugin, we might want to be more careful when making changes in the future.

The same as you suggested, we put nil as the deck path to fix it, and now everything is fine.

[pingsutw]

@honnix got it, sorry. we'll be more careful when making changes to public API in the future.

[honnix]

@pingsutw No worries. It was easy to catch and fix. :)