flyteorg / flyte

Scalable and flexible workflow orchestration platform that seamlessly unifies data, ML and analytics stacks.
https://flyte.org
Apache License 2.0
5.8k stars 660 forks source link

[BUG] Flytekit entity patch command interferes with mock.patch #854

Open wild-endeavor opened 3 years ago

wild-endeavor commented 3 years ago

Describe the bug This short test file will reproduce the issue. If you copy paste this into a test file, each of the two tests will succeed on its own. However if you run it together as a file in a pytest command, it will break.

import pytest
from mock import patch as _system_patch

from flytekit.core.task import task
from flytekit.core.testing import patch as flyte_patch
from flytekit.core.workflow import ImperativeWorkflow, workflow

@task
def t1(a: str) -> str:
    return a + " world"

wb = ImperativeWorkflow(name="my.workflow")
wb.add_workflow_input("in1", str)
node = wb.add_entity(t1, a=wb.inputs["in1"])
wb.add_workflow_output("from_n0t1", node.outputs["o0"])

@_system_patch("flytekit.core.workflow.ImperativeWorkflow.execute")
def test_return_none_errors(mock_execute):
    mock_execute.return_value = None
    with pytest.raises(Exception):
        wb(in1="hello")

@flyte_patch(wb)
def test_imperative_patching(mock_wb):
    mock_wb.return_value = "hi"

    @workflow
    def my_functional_wf(a: str) -> str:
        x = wb(in1=a)
        return x

    assert my_functional_wf(a="hello") == "hi"

It is unclear what is happening. Somehow, the flyte_patch patching logic is interfering with the normal mock.patch. The mere presence of the second test, basically makes it seem like the mock.patch isn't there on the first test. Basically ImperativeWorkflow.execute is not patched and returns what it normally would return, and hence the exception isn't raised.

If you copy paste the wb workflow and use a new variable for it for the second test, then things pass. This will take some digging into the mock library to understand how they're interfering with each other.

github-actions[bot] commented 1 year ago

Hello 👋, This issue has been inactive for over 9 months. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will close the issue if we detect no activity in the next 7 days. Thank you for your contribution and understanding! 🙏

github-actions[bot] commented 1 year ago

Hello 👋, This issue has been inactive for over 9 months and hasn't received any updates since it was marked as stale. We'll be closing this issue for now, but if you believe this issue is still relevant, please feel free to reopen it. Thank you for your contribution and understanding! 🙏

github-actions[bot] commented 3 months ago

Hello 👋, this issue has been inactive for over 9 months. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will engage on it to decide if it is still applicable. Thank you for your contribution and understanding! 🙏