Streamline __call__ pattern between Tasks and Workflows

[wild-endeavor]

Signed-off-by: wild-endeavor [email protected]

TL;DR

Please read through the description in the issue as it's pretty succinct.

Note that this PR only touches local execution. Production runs on a real Flyte backend is not touched.

Description

This PR basically further streamlines the logic in the __call__ function of tasks and workflows. At this point the logic is pretty much identical. (But premature OOP inheritance pattern has bitten us in the past so we'll refrain from creating a base class out of it for now.)

Unit Test Changes

We had to make some changes to the unit test to get this new paradigm to work. You likely will need to make similar changes to your tests.

1. Dataframe handling: If your task returned an instance of a Pandas dataframe as a FlyteSchema type in the past, since the Type Engine wasn't triggered, you just got the raw pandas dataframe. Now you'll get the FlyteSchema object, so you'll need to change how you read that. (See the change in tests/flytekit/unit/core/test_type_hints.py)
2. Sagemaker tasks: Probably nothing needs to happen but if you had modified sagemaker tasks to be tested locally, you'll need to provide real directories to the train and validation arguments.

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

Tracking Issue

flyteorg/flyte#1032

[codecov]

Codecov Report

Merging #588 (0772812) into master (86d3368) will increase coverage by 0.04%.
The diff coverage is 85.48%.

@@            Coverage Diff             @@
##           master     #588      +/-   ##
==========================================
+ Coverage   85.57%   85.61%   +0.04%     
==========================================
  Files         379      379              
  Lines       29685    29677       -8     
  Branches     2376     2368       -8     
==========================================
+ Hits        25403    25409       +6     
+ Misses       3640     3632       -8     
+ Partials      642      636       -6     

Impacted Files	Coverage Δ
flytekit/core/node_creation.py	67.27% <ø> (ø)
flytekit/core/python_function_task.py	85.57% <ø> (-1.93%)	⬇️
flytekit/core/promise.py	77.03% <75.00%> (+0.08%)	⬆️
flytekit/core/base_task.py	89.15% <100.00%> (+0.89%)	⬆️
flytekit/core/reference_entity.py	91.91% <100.00%> (ø)
flytekit/core/workflow.py	89.66% <100.00%> (+1.95%)	⬆️
...ugins/flytekit-k8s-pod/flytekitplugins/pod/task.py	93.15% <100.00%> (ø)
plugins/tests/awssagemaker/test_hpo.py	100.00% <100.00%> (ø)
plugins/tests/awssagemaker/test_training.py	100.00% <100.00%> (ø)
tests/flytekit/unit/core/test_type_hints.py	96.13% <100.00%> (ø)
... and 7 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 86d3368...0772812. Read the comment docs.

[kumare3]

is the expected outputs check new?

[wild-endeavor]

it is - copied from the workflow __call__ function - just making things as similar as possible.

[kumare3]

@wild-endeavor I think we probably should make the common call logic for both Workflow and tasks. But, if possible as you suggest in this comment

This PR basically further streamlines the logic in the call function of tasks and workflows. At this point the logic is pretty much identical. (But premature OOP inheritance pattern has bitten us in the past so we'll refrain from creating a base class out of it for now.)

We should maybe just make it a standalone method that does the dataconversion and invocation. That way the logic is pretty nicely isolated and yet shared?

[kumare3]

And if I did not mention - I think this makes a ton of sense - this makes all flyte transformations always required which is perfect

[wild-endeavor]

@jeevb thoughts?

[kumare3]

the logic is the same for workflows and tasks, but we did not want to create base class, hence a separate method