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

[Docs] Prevent using mutable default arguments in flytesnacks #5383

Open
2 tasks done
MortalHappiness opened this issue May 17, 2024 · 3 comments · May be fixed by flyteorg/flytesnacks#1685
Open
2 tasks done

[Docs] Prevent using mutable default arguments in flytesnacks #5383

MortalHappiness opened this issue May 17, 2024 · 3 comments · May be fixed by flyteorg/flytesnacks#1685
Labels
documentation Improvements or additions to documentation flytesnacks good first issue Good for newcomers

Comments

@MortalHappiness
Copy link
Contributor

Description

Mutable default arguments is a common anti-pattern in Python. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments for details. We should prevent using it in flytesnacks.

For example:

https://github.com/flyteorg/flytesnacks/blob/2d73e2a0892a56efc4fe3649db942e8270ccfe8c/examples/advanced_composition/advanced_composition/map_task.py#L13

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@MortalHappiness MortalHappiness added documentation Improvements or additions to documentation untriaged This issues has not yet been looked at by the Maintainers labels May 17, 2024
Copy link

runllm bot commented May 17, 2024

Description

Mutable default arguments is a common anti-pattern in Python. See Mutable Default Arguments for details. We should prevent using it in flytesnacks.

For example:

https://github.com/flyteorg/flytesnacks/blob/2d73e2a0892a56efc4fe3649db942e8270ccfe8c/examples/advanced_composition/advanced_composition/map_task.py#L13

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes

Sources:


This is an AI-generated response and your feedback is appreciated! Please leave a 👍 if this is helpful and 👎 if it is not.

Mention @RunLLM to ask follow-up questions. See runllm.com for more!

@pingsutw pingsutw added good first issue Good for newcomers flytesnacks and removed untriaged This issues has not yet been looked at by the Maintainers labels May 17, 2024
@eapolinario
Copy link
Contributor

@MortalHappiness , please see flyteorg/flytesnacks#1685 (comment) and let me know what you think.

OTOH, enabling this in flytekit should be a no-brainer.

@MortalHappiness
Copy link
Contributor Author

MortalHappiness commented Jun 5, 2024

@eapolinario I still don't think it is a good pattern, especially in flytesnacks, which is the documentation for examples. Examples with anti-patterns may mislead users that are not proficient in Python to write these code.

Actually, I found this problem when I was working on this PR:
flyteorg/flytekit#2401

I know that if the default argument is used in workflow like the following:

@task
def t1(a: list[int]) -> list[int]:
    return a


@workflow
def wf(a: list[int] = [1, 2, 3]) -> list[int]:
    a.append(4)
    return t1(a=a)

It will show that TypeError: 'Promise' object is not callable. Therefore the list will not be modified.

However, in the PR that I mentioned above, I need to handle the case of @dynamic and @task. Therefore, users may write the following code

@task
def t1(a: list[int] = [1, 2, 3]) -> list[int]:
    a.append(4)
    return a

@workflow
def wf() -> list[int]:
    t1()
    return t1()

Note that t1 is called twice. In this case, should the result of wf be [1, 2, 3, 4] or [1, 2, 3, 4, 4]? If we make the list immutable, then the result should be [1, 2, 3, 4], which causes no problems. But in Python's perspective, although this is an anti-pattern, the "correct" result should be [1, 2, 3, 4, 4]. If users are migrated from an existing Python project, they may be confusing why the result of wf is different after adding the @task and @workflow decorator. Therefore, in my PR, if the default argument is a non-hashable object and no inputs are provided, a FlyteAssertion is raised instead to prevent this anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation flytesnacks good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants