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

Throw warning for nested @Task functions #1727

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Conversation

oliverhu
Copy link
Contributor

@oliverhu oliverhu commented Jul 8, 2023

TL;DR

Address flyteorg/flyte#3734 that nested Tasks/Workflows is not warned.

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

In order to detect entities that are nested inside @task:

  1. During local execution, create a new FlyteContext with LOCAL_TASK_EXECUTION for @task,and LOCAL_WORKFLOW_EXECUTION if it is @workflow.
  2. If flyte_entity_call_handler is invoked inside LOCAL_TASK_EXECUTION or TASK_EXECUTION, that means it is called inside @task, warn the users of nested @tasks/@workflows.

Created a helper function is_local_execution() to shorthand the checks.

Tracking Issue

flyteorg/flyte#3734

Follow-up issue

The call() function in remote_callable and reference_entity duplicates and can be replaced with flyte_entity_call_handler as well in the future.

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Others LGTM

flytekit/remote/remote_callable.py Outdated Show resolved Hide resolved
oliverhu and others added 2 commits July 14, 2023 09:26
pingsutw
pingsutw previously approved these changes Jul 18, 2023
flytekit/core/promise.py Outdated Show resolved Hide resolved
@ByronHsu
Copy link
Collaborator

Also, i would prefer to throw exceptions instead of merely logging errors because a nested task is illegal when running remotely?

@ByronHsu
Copy link
Collaborator

Just checked with @pingsutw. The nested workflow can actually run fine remotely. To keep backward compatibility, let's just stick with logger.

ByronHsu
ByronHsu previously approved these changes Jul 24, 2023
pingsutw
pingsutw previously approved these changes Jul 24, 2023
Signed-off-by: oliverhu <[email protected]>
@oliverhu oliverhu dismissed stale reviews from pingsutw and ByronHsu via a7e67a9 July 25, 2023 00:05
@pingsutw
Copy link
Member

merged, thanks @oliverhu @ByronHsu

@pingsutw pingsutw merged commit 31c41ed into flyteorg:master Jul 25, 2023
@welcome
Copy link

welcome bot commented Jul 25, 2023

Congrats on merging your first pull request! 🎉

fg91 pushed a commit that referenced this pull request Aug 15, 2023
* Throw warning for nested @task functions

Signed-off-by: oliverhu <[email protected]>

* Update flytekit/remote/remote_callable.py

Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: oliverhu <[email protected]>

* also update execution state for default case

Signed-off-by: oliverhu <[email protected]>

* fix linting

Signed-off-by: oliverhu <[email protected]>

---------

Signed-off-by: oliverhu <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
@fellhorn
Copy link
Contributor

There seems to be a bug in this implementation as it throws a warning for valid dynamic workflows:

from flytekit import workflow, dynamic, task

@task
def my_task():
    print("foo")

@dynamic
def outer():
    my_task()


@workflow
def workflow(
):
    outer()

if __name__ == "__main__":
    workflow()

prints:

{"asctime": "2023-08-29 11:57:57,824", "name": "flytekit", "levelname": "ERROR", "message": "You are not supposed to nest @Task/@Workflow inside a @Task!"}
foo

@fellhorn
Copy link
Contributor

fellhorn commented Aug 29, 2023

@fg91 and I quickly debugged the promise.py where we log this error message and could not find a simple solution to detect that an @task is called within an @dynamic instead of an @task.

Therefore we created an issue here to continue the discussion: flyteorg/flyte#3995

@oliverhu
Copy link
Contributor Author

Let's follow up in flyteorg/flyte#3995

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.

4 participants