Skip to content

Don't node sync on remote wait - #690

Merged
eapolinario merged 2 commits into
masterfrom
remote-simple-sync-on-wait
Oct 7, 2021
Merged

Don't node sync on remote wait#690
eapolinario merged 2 commits into
masterfrom
remote-simple-sync-on-wait

Conversation

@wild-endeavor

@wild-endeavor wild-endeavor commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

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

TL;DR

Don't node sync on remote wait.

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

https://flyte-org.slack.com/archives/CREL4QVAQ/p1633567961295600

@wild-endeavor
no node sync
6f3d405 
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
vsbus
vsbus reviewed Oct 7, 2021
View reviewed changes
Comment thread flytekit/remote/remote.py Outdated

while datetime.utcnow() < time_to_give_up:
execution = self.sync_workflow_execution(execution)
execution = self.sync_workflow_execution(execution, sync_nodes=False)

@vsbus vsbus Oct 7, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if you do it, then no wait() calls will be fully synced and user will need to do it manually after .wait().

It might be better to make sync_nodes=True a parameter of wait() and let users use False for hacky workarounds. Also might be good to remove it later when hack is not needed.

@wild-endeavor wild-endeavor Oct 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure.

@wild-endeavor
add param to wait
80d646f 
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
vsbus
vsbus approved these changes Oct 7, 2021
View reviewed changes

@vsbus vsbus left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍
Thank you!

@codecov

codecov Bot commented Oct 7, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #690 (80d646f) into master (7f03137) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #690   +/-   ##
=======================================
  Coverage   85.68%   85.68%           
=======================================
  Files         355      355           
  Lines       29684    29684           
  Branches     2426     2426           
=======================================
  Hits        25436    25436           
  Misses       3606     3606           
  Partials      642      642
Impacted Files Coverage Δ
flytekit/remote/remote.py 72.46% <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 7f03137...80d646f. Read the comment docs.

@eapolinario
eapolinario merged commit 5d28829 into master Oct 7, 2021
eapolinario pushed a commit that referenced this pull request Oct 8, 2021
@wild-endeavor @eapolinario
Don't node sync on remote wait (#690)
2887216 
* 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>
AdrianoKF pushed a commit to AdrianoKF/flytekit that referenced this pull request Oct 11, 2021
@wild-endeavor @AdrianoKF
Don't node sync on remote wait (flyteorg#690)
fe02c81 
* 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>
eapolinario added a commit that referenced this pull request Oct 19, 2021
@eapolinario @pingsutw
@wild-endeavor @cosmicbboy
Add official support for python 3.9 (#687)
ae15c4e 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eapolinario eapolinario eapolinario approved these changes
@katrogan katrogan Awaiting requested review from katrogan
@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner
+1 more reviewer
@vsbus vsbus vsbus approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wild-endeavor @eapolinario @vsbus