-
Notifications
You must be signed in to change notification settings - Fork 299
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Others LGTM
Signed-off-by: oliverhu <[email protected]>
Co-authored-by: Kevin Su <[email protected]> Signed-off-by: oliverhu <[email protected]>
Also, i would prefer to throw exceptions instead of merely logging errors because a nested task is illegal when running remotely? |
Signed-off-by: oliverhu <[email protected]>
Just checked with @pingsutw. The nested workflow can actually run fine remotely. To keep backward compatibility, let's just stick with logger. |
Signed-off-by: oliverhu <[email protected]>
Congrats on merging your first pull request! 🎉 |
* 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]>
There seems to be a bug in this implementation as it throws a warning for valid dynamic workflows:
prints:
|
@fg91 and I quickly debugged the Therefore we created an issue here to continue the discussion: flyteorg/flyte#3995 |
Let's follow up in flyteorg/flyte#3995 |
TL;DR
Address flyteorg/flyte#3734 that nested Tasks/Workflows is not warned.
Type
Are all requirements met?
Complete description
In order to detect entities that are nested inside
@task
:FlyteContext
withLOCAL_TASK_EXECUTION
for@task
,andLOCAL_WORKFLOW_EXECUTION
if it is@workflow
.flyte_entity_call_handler
is invoked insideLOCAL_TASK_EXECUTION
orTASK_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.