Skip to content
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

Serialize directly to model classes #481

Merged
merged 17 commits into from
May 21, 2021
Merged

Serialize directly to model classes #481

merged 17 commits into from
May 21, 2021

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented May 19, 2021

TL;DR

Gets rid of the Sdk... classes in the translator.py module. This should be the last place flytekit is explicitly using the old classes.

Previously the translator module converted from the new Python classes (PythonFunctionTask, PythonFunctionWorkflow, LaunchPlan) into the SdkXyz classes, and then relied on the serialize() call within each of those Sdk classes, invoked by serialize.py to convert them into model objects. Now translator will go directly to the model, and the serialization step will just call to_flyte_idl()

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

  • Remove most of the common dependencies in translator.py and replace with the model classes.
  • Python objects get converted to the same model classes as the output of their respective serialize calls earlier, namely
    • Tasks -> flytekit.models.task.TaskSpec
    • Launch Plans -> flytekit.models.launch_plan.LaunchPlan
    • Workflows -> flytekit.models.admin.WorkflowSpec
  • Realized that we were actually still relying on the two AuthRole settings in the serialization step. These were being read in the property field for auth_role in the SdkLaunchPlan class itself.
  • Handling for reference tasks/workflows is actually not great when it comes to getting the model representation. I'm currently using None. That is, whenever a reference entity is run through translator.py, a None is returned and cached in the ordered dict. This is useful because the translator is used in one other place - in dynamic tasks. The runtime dynamic task compilation will look for these Nones and reject them. Using None as a sentinel value is not good, but I think this PR has enough going on so will address that in a future PR.

Tracking Issue

flyteorg/flyte#822

Follow-up issue

flyteorg/flyte#1044

Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #481 (c29ffb4) into master (92290b9) will decrease coverage by 0.03%.
The diff coverage is 90.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
- Coverage   84.59%   84.56%   -0.04%     
==========================================
  Files         363      363              
  Lines       27200    27269      +69     
  Branches     2237     2247      +10     
==========================================
+ Hits        23011    23061      +50     
- Misses       3583     3605      +22     
+ Partials      606      603       -3     
Impacted Files Coverage Δ
flytekit/clis/flyte_cli/main.py 43.53% <0.00%> (ø)
flytekit/core/condition.py 91.66% <ø> (-0.05%) ⬇️
flytekit/clis/sdk_in_container/serialize.py 39.79% <25.92%> (-0.21%) ⬇️
flytekit/common/translator.py 89.14% <89.55%> (-0.94%) ⬇️
flytekit/core/python_function_task.py 89.01% <92.30%> (+2.77%) ⬆️
tests/flytekit/unit/core/test_launch_plan.py 92.79% <96.00%> (+0.83%) ⬆️
tests/flytekit/unit/core/test_references.py 97.27% <96.07%> (-0.65%) ⬇️
flytekit/core/node.py 84.61% <100.00%> (+2.98%) ⬆️
plugins/tests/hive/test_hive_task.py 100.00% <100.00%> (ø)
plugins/tests/pod/test_pod.py 95.45% <100.00%> (ø)
... and 19 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 92290b9...c29ffb4. Read the comment docs.

Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
@wild-endeavor wild-endeavor marked this pull request as ready for review May 20, 2021 17:30
@wild-endeavor wild-endeavor changed the title Idl direct Serialize directly to model classes May 20, 2021
@@ -1818,6 +1818,7 @@ def _extract_and_register(

flyte_entities_list = _extract_files(project, domain, version, file_paths, patches)
for id, flyte_entity in flyte_entities_list:
_click.secho(f"Registering {id}", fg="green")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_click.secho(f"Registering {id}", fg="green")
_click.secho(f"Registering {id}", fg="yellow")

Copy link
Contributor

Choose a reason for hiding this comment

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

green makes you feel that it was successful.
Ideally I would like this

Registering {name}..... [OK] | [FAILED]

@kumare3
Copy link
Contributor

kumare3 commented May 20, 2021

this looks awesome

kumare3
kumare3 previously approved these changes May 20, 2021
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
Signed-off-by: wild-endeavor <[email protected]>
@kumare3 kumare3 self-requested a review May 21, 2021 18:13
kumare3
kumare3 previously approved these changes May 21, 2021
Signed-off-by: wild-endeavor <[email protected]>
@kumare3 kumare3 self-requested a review May 21, 2021 18:45
@wild-endeavor wild-endeavor merged commit e373b50 into master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants