Remove sync singledispatch, add option for top-level only sync by wild-endeavor · Pull Request #681 · flyteorg/flytekit

wild-endeavor · 2021-09-29T19:57:23Z

Signed-off-by: Yee Hing Tong wild-endeavor@users.noreply.github.com

TL;DR

This removes the singledispatchmethod for remote.sync and replaces the main function with one that only works for FlyteWorkflowExecution instances. The other registered methods have been renamed to sync_xyz. The singledispatch was too hard to decipher and confusing to navigate in an IDE.

Also added a switch sync_nodes (by default True to maintain current behavior) to the sync call so that people can sync executions that have dynamic tasks that run launch plans and subworkflows in lieu of flyteorg/flyte#1482

Type

Bug Fix
Feature
Plugin
Refactor

Are all requirements met?

Complete description

See above.

Tracking Issue

NA

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

codecov · 2021-09-29T20:20:12Z

Codecov Report

Merging #681 (1c5697c) into master (243adb7) will decrease coverage by 0.01%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage   85.69%   85.68%   -0.02%     
==========================================
  Files         355      355              
  Lines       29683    29682       -1     
  Branches     2425     2427       +2     
==========================================
- Hits        25438    25434       -4     
- Misses       3604     3606       +2     
- Partials      641      642       +1

Impacted Files	Coverage Δ
flytekit/remote/remote.py	`72.46% <55.55%> (-0.79%)`	⬇️
tests/flytekit/integration/remote/test_remote.py	`91.62% <100.00%> (ø)`

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 243adb7...1c5697c. Read the comment docs.

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

kumare3

Why can't we still keep the sync method and just use an if else case to switch dispatch?

eapolinario · 2021-10-04T18:07:17Z

Why can't we still keep the sync method and just use an if else case to switch dispatch?

I'd argue that this makes the code less clear, I'd much rather keep sync in the API for backwards compatibility (people in the wild are already relying on this) and make the library code more clear. In the future we might deprecate sync altogether if we can reach a point where it's not needed (i.e. we'll sync whenever necessary, still TBD if we can do it in all cases without taking a huge perf hit).

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

eapolinario · 2021-10-06T23:03:00Z

+        so that we don't break people.
+        :param execution:
+        :param entity_definition:
+        :return:


can you update the docstring?

eapolinario · 2021-10-06T23:05:02Z

-        execution: typing.Union[FlyteWorkflowExecution, FlyteNodeExecution, FlyteTaskExecution],
+        execution: FlyteWorkflowExecution,
        entity_definition: typing.Union[FlyteWorkflow, FlyteTask] = None,
+        workflow_only: bool = False,


workflow_only is not clear, how about sync_children_nodes?

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

eapolinario

LGTM

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com> Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

…org#681) Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* Run 3.9 in CI Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Also run 3.9 in plugins tests. Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Fix tests Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Account for the different exception type raised in case of wrong types Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Exclude spark2 Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Failed to load json_data to dataclass (#684) Signed-off-by: Kevin Su <pingsutw@apache.org> Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Remove sync singledispatch, add option for top-level only sync (#681) Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com> Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Failed to transform path string to Literal (#689) Signed-off-by: Kevin Su <pingsutw@apache.org> Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Don't node sync on remote wait (#690) * no node sync Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com> * add param to wait Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com> Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Fix plugin regressions (#688) * fix pandera regression Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * install plugin with pip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * fix pandera plugin tests Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * add spark flytekit plugin to papermill test_requires Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * add sqlalchemy to great expectations plugin Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * wip Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * plugins plugins plugins! Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * lint Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com> * Exclude does not understand lists Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Invert python version check Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Invert python version check for real this time Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Enable 3.10 just for kicks Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Add quotes around python versions Yes, this is needed, please see https://dev.to/hugovk/the-python-3-1-problem-85g. Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Revert "Add quotes around python versions" This reverts commit 4d619d5. Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Revert "Enable 3.10 just for kicks" This reverts commit bd6d694. Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * wip - restricted types Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * wip - restricted types Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Comment use of restricted types in get_transformer Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Publish 3.9 image Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Add python 3.9 to the list of supported languages Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Comment RestrictedTypeTransformer and add one test case. Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Review feedback Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Comment RestrictedTypeTransformer Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Handle the TypeError Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Remove breakpoint Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> Co-authored-by: Kevin Su <pingsutw@gmail.com> Co-authored-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com> Co-authored-by: Niels Bantilan <niels.bantilan@gmail.com>

remove sync singledispatch

7954019

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

Add sync method back

7c2cf56

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

eapolinario marked this pull request as ready for review October 1, 2021 21:33

eapolinario requested review from katrogan and kumare3 as code owners October 1, 2021 21:33

kumare3 reviewed Oct 2, 2021

View reviewed changes

wild-endeavor added 4 commits October 6, 2021 13:54

merge master and add back top level sync

25b0bbf

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

add option

e6d8b5b

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

pull

aeea702

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

add option to top level sync

1dbb8c6

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

wild-endeavor changed the title ~~Remove sync singledispatch~~ Remove sync singledispatch, add option for top-level only sync Oct 6, 2021

eapolinario reviewed Oct 6, 2021

View reviewed changes

pr comment

1c5697c

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

eapolinario approved these changes Oct 6, 2021

View reviewed changes

wild-endeavor merged commit 1fb425f into master Oct 7, 2021

AdrianoKF pushed a commit to AdrianoKF/flytekit that referenced this pull request Oct 11, 2021

Remove sync singledispatch, add option for top-level only sync (flyte…

d99fcec

…org#681) Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

Navigation Menu

Remove sync singledispatch, add option for top-level only sync - #681

Remove sync singledispatch, add option for top-level only sync#681
wild-endeavor merged 7 commits into
masterfrom
clean-up-remote

wild-endeavor commented Sep 29, 2021 •

edited

Loading

Uh oh!

codecov Bot commented Sep 29, 2021 •

edited

Loading

Uh oh!

kumare3 left a comment

Uh oh!

eapolinario commented Oct 4, 2021

Uh oh!

eapolinario Oct 6, 2021

Uh oh!

eapolinario Oct 6, 2021

Uh oh!

wild-endeavor Oct 6, 2021

Uh oh!

eapolinario left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wild-endeavor commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Uh oh!

codecov Bot commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kumare3 left a comment

Choose a reason for hiding this comment

Uh oh!

eapolinario commented Oct 4, 2021

Uh oh!

eapolinario Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

eapolinario Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

wild-endeavor Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

eapolinario left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wild-endeavor commented Sep 29, 2021 •

edited

Loading

codecov Bot commented Sep 29, 2021 •

edited

Loading