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

Decouple scripts from WorkContext #609

Merged
merged 38 commits into from
Sep 9, 2021
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
fef588d
Create Script class, move relevant logic to its package
kmazurek Aug 19, 2021
9484671
WIP: add evaluation to script steps
kmazurek Aug 19, 2021
b767676
WIP: add new_script to WorkContext
kmazurek Aug 23, 2021
22373aa
Add timeout and wait_for_results to Script init
kmazurek Aug 24, 2021
9b5010b
Make script commands a public interface
kmazurek Aug 24, 2021
0351664
Enable awaiting individual commands of a script
kmazurek Aug 26, 2021
5f385c3
Make Script._serialize synchronous
kmazurek Aug 26, 2021
efaf2c3
Add direct step calls to Script
kmazurek Aug 26, 2021
6f5fcfd
Make WorkContext backward compatible with Script
kmazurek Aug 26, 2021
2af95d3
Merge branch 'master' into km/context-script-decouple
kmazurek Aug 27, 2021
3742e18
Fix isinstance check in _set_cmd_result
kmazurek Aug 30, 2021
063c684
Update type hint on *args
kmazurek Aug 30, 2021
8278256
Adapt WorkContext unit tests
kmazurek Aug 30, 2021
6a95fc7
Add Script unit tests
kmazurek Aug 31, 2021
3d9c123
WIP: test agr termination debugging
kmazurek Sep 1, 2021
cc27a0a
Fix script handling in process_batches
kmazurek Sep 1, 2021
9d7f2aa
Update Script unit tests
kmazurek Sep 1, 2021
d8430ed
Update types based on mypy
kmazurek Sep 1, 2021
92775fc
Remove unused classes
kmazurek Sep 1, 2021
8214af1
Address flake8 errors
kmazurek Sep 1, 2021
69150eb
Fix AsyncMock imports for Python <3.8
kmazurek Sep 1, 2021
f788d54
Merge branch 'master' into km/context-script-decouple
kmazurek Sep 1, 2021
8dfea06
Add missing docstrings
kmazurek Sep 1, 2021
3386271
Merge branch 'master' into km/context-script-decouple
kmazurek Sep 2, 2021
f503432
Add Script docstring
kmazurek Sep 2, 2021
5378db3
Reverse order of stdout and stderr in Run#__init__
kmazurek Sep 3, 2021
82d2cd7
Change Script fields to be instance variables
kmazurek Sep 3, 2021
4281648
Add id property to Script instances
kmazurek Sep 3, 2021
c086011
Move command result future to Command field
kmazurek Sep 7, 2021
e574447
Update type hint on Command result
kmazurek Sep 7, 2021
fbb32b1
Add return statements to WorkContext command methods
kmazurek Sep 8, 2021
53d23eb
Add a factory for WorkContext objects in tests
kmazurek Sep 8, 2021
4741009
Rename send_* methods to upload_* on Script
kmazurek Sep 8, 2021
37f3dfe
Make stdout and stderr optional on Script.run
kmazurek Sep 8, 2021
64f4a17
Remove redundant condition from Run.evaluate
kmazurek Sep 8, 2021
804f765
Update yapapi/script/__init__.py
kmazurek Sep 8, 2021
b9f4d32
Merge branch 'master' into km/context-script-decouple
kmazurek Sep 8, 2021
41d731e
Merge branch 'master' into km/context-script-decouple
kmazurek Sep 9, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions tests/script/test_script.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
from functools import partial
import json
import pytest
import sys
from unittest import mock

from yapapi import WorkContext
from yapapi.events import CommandExecuted
from yapapi.script import Script
from yapapi.script.command import Deploy, Start


@pytest.mark.skipif(sys.version_info < (3, 8), reason="AsyncMock requires python 3.8+")
class TestScript:
@pytest.fixture(autouse=True)
def setUp(self):
self._on_download_executed = False

@pytest.fixture
def work_context(self):
return WorkContext(mock.MagicMock(), mock.MagicMock(), storage=mock.AsyncMock())
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think this could be converted into a factoryboy factory so that it's reusable anywhere else where we'd like to have a mock WorkContext....

Copy link
Contributor Author

@kmazurek kmazurek Sep 8, 2021

Choose a reason for hiding this comment

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

Done in 53d23eb


@staticmethod
def _assert_dst_path(script: Script, dst_path):
batch = script._evaluate()
# transfer_cmd = {'transfer': {'from': 'some/mock/path', 'to': 'container:expected/path'}
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover cruft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a helper comment to show what transfer_cmd should look like. I see how it can be confusing, I'm going to remove it.

Copy link
Contributor Author

@kmazurek kmazurek Sep 8, 2021

Choose a reason for hiding this comment

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

Removed in 53d23eb

transfer_cmd = [cmd for cmd in batch if "transfer" in cmd][0]
assert transfer_cmd["transfer"]["to"] == f"container:{dst_path}"

@staticmethod
def _assert_src_path(script: Script, src_path):
batch = script._evaluate()
# transfer_cmd = {'transfer': {'from': 'container:expected/path', 'to': 'some/mock/path'}
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover cruft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

@kmazurek kmazurek Sep 8, 2021

Choose a reason for hiding this comment

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

Removed in 53d23eb

transfer_cmd = [cmd for cmd in batch if "transfer" in cmd][0]
assert transfer_cmd["transfer"]["from"] == f"container:{src_path}"

async def _on_download(self, expected, data: bytes):
assert data == expected
self._on_download_executed = True

@pytest.mark.asyncio
async def test_send_json(self, work_context: WorkContext):
storage: mock.AsyncMock = work_context._storage
dst_path = "/test/path"
data = {
"param": "value",
}

script = work_context.new_script()
script.send_json(data, dst_path)
await script._before()

storage.upload_bytes.assert_called_with(json.dumps(data).encode("utf-8"))
self._assert_dst_path(script, dst_path)

@pytest.mark.asyncio
async def test_send_bytes(self, work_context: WorkContext):
storage: mock.AsyncMock = work_context._storage
dst_path = "/test/path"
data = b"some byte string"

script = work_context.new_script()
script.send_bytes(data, dst_path)
await script._before()

storage.upload_bytes.assert_called_with(data)
self._assert_dst_path(script, dst_path)

@pytest.mark.asyncio
async def test_download_bytes(self, work_context: WorkContext):
expected = b"some byte string"
storage: mock.AsyncMock = work_context._storage
storage.new_destination.return_value.download_bytes.return_value = expected
src_path = "/test/path"

script = work_context.new_script()
script.download_bytes(src_path, partial(self._on_download, expected))
await script._before()
await script._after()

self._assert_src_path(script, src_path)
assert self._on_download_executed

@pytest.mark.asyncio
async def test_download_json(self, work_context: WorkContext):
expected = {"key": "val"}
storage: mock.AsyncMock = work_context._storage
storage.new_destination.return_value.download_bytes.return_value = json.dumps(
expected
).encode("utf-8")
src_path = "/test/path"

script = work_context.new_script()
script.download_json(src_path, partial(self._on_download, expected))
await script._before()
await script._after()

self._assert_src_path(script, src_path)
assert self._on_download_executed

@pytest.mark.asyncio
async def test_implicit_init(self, work_context: WorkContext):
script = work_context.new_script()

# first script, should include implicit deploy and start cmds
await script._before()
assert len(script._commands) == 2
deploy_cmd, _ = script._commands[0]
assert isinstance(deploy_cmd, Deploy)
start_cmd, _ = script._commands[1]
assert isinstance(start_cmd, Start)
assert work_context._started

# second script, should not include implicit deploy and start
script = work_context.new_script()
script.run("/some/cmd")
await script._before()
assert len(script._commands) == 1

@pytest.mark.asyncio
async def test_cmd_result(self, work_context: WorkContext):
script = work_context.new_script()
future_result = script.run("/some/cmd", 1)

await script._before()
run_cmd, _ = script._commands[2]
result = CommandExecuted(
"job_id", "agr_id", "script_id", 2, command=run_cmd.evaluate(work_context)
)
script._set_cmd_result(result)

assert future_result.done()
assert future_result.result() == result
77 changes: 40 additions & 37 deletions tests/test_ctx.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from unittest import mock

from yapapi.ctx import CommandContainer, WorkContext
from yapapi.script import Script


def test_command_container():
Expand Down Expand Up @@ -37,6 +38,7 @@ def test_command_container():
assert json.loads(expected_commands) == c.commands()


@pytest.mark.skipif(sys.version_info < (3, 8), reason="AsyncMock requires python 3.8+")
class TestWorkContext:
@pytest.fixture(autouse=True)
def setUp(self):
Expand All @@ -47,82 +49,85 @@ def _get_work_context(storage=None):
return WorkContext(mock.Mock(), mock.Mock(), storage=storage)

@staticmethod
def _assert_dst_path(steps, dst_path):
c = CommandContainer()
steps.register(c)
assert c.commands().pop()["transfer"]["to"] == f"container:{dst_path}"
def _assert_dst_path(script: Script, dst_path):
batch = script._evaluate()
# transfer_cmd = {'transfer': {'from': 'some/mock/path', 'to': 'container:expected/path'}
Copy link
Contributor

Choose a reason for hiding this comment

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

same leftover cruft...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not cruft!

Copy link
Contributor Author

@kmazurek kmazurek Sep 8, 2021

Choose a reason for hiding this comment

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

Removed anyway in 53d23eb

transfer_cmd = [cmd for cmd in batch if "transfer" in cmd][0]
assert transfer_cmd["transfer"]["to"] == f"container:{dst_path}"

@staticmethod
def _assert_src_path(steps, src_path):
c = CommandContainer()
steps.register(c)
assert c.commands().pop()["transfer"]["from"] == f"container:{src_path}"
def _assert_src_path(script: Script, src_path):
batch = script._evaluate()
# transfer_cmd = {'transfer': {'from': 'container:expected/path', 'to': 'some/mock/path'}
Copy link
Contributor

Choose a reason for hiding this comment

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

same leftover cruft...

Copy link
Contributor Author

@kmazurek kmazurek Sep 8, 2021

Choose a reason for hiding this comment

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

Removed in 53d23eb

transfer_cmd = [cmd for cmd in batch if "transfer" in cmd][0]
assert transfer_cmd["transfer"]["from"] == f"container:{src_path}"

async def _on_download(self, expected, data: bytes):
assert data == expected
self._on_download_executed = True

@pytest.mark.asyncio
@pytest.mark.skipif(sys.version_info < (3, 8), reason="AsyncMock requires python 3.8+")
async def test_send_json(self):
storage = mock.AsyncMock()
dst_path = "/test/path"
data = {
"param": "value",
}
ctx = self._get_work_context(storage)

ctx.send_json(dst_path, data)
steps = ctx.commit()
await steps.prepare()
script = ctx.commit()
await script._before()

storage.upload_bytes.assert_called_with(json.dumps(data).encode("utf-8"))
self._assert_dst_path(steps, dst_path)
self._assert_dst_path(script, dst_path)

@pytest.mark.asyncio
@pytest.mark.skipif(sys.version_info < (3, 8), reason="AsyncMock requires python 3.8+")
async def test_send_bytes(self):
storage = mock.AsyncMock()
dst_path = "/test/path"
data = b"some byte string"
ctx = self._get_work_context(storage)

ctx.send_bytes(dst_path, data)
steps = ctx.commit()
await steps.prepare()
script = ctx.commit()
await script._before()

storage.upload_bytes.assert_called_with(data)
self._assert_dst_path(steps, dst_path)
self._assert_dst_path(script, dst_path)

@pytest.mark.asyncio
@pytest.mark.skipif(sys.version_info < (3, 8), reason="AsyncMock requires python 3.8+")
async def test_download_bytes(self):
expected = b"some byte string"

storage = mock.AsyncMock()
storage.new_destination.return_value.download_bytes.return_value = expected

src_path = "/test/path"
ctx = self._get_work_context(storage)

ctx.download_bytes(src_path, partial(self._on_download, expected))
steps = ctx.commit()
await steps.prepare()
await steps.post()
self._assert_src_path(steps, src_path)
script = ctx.commit()
await script._before()
await script._after()

self._assert_src_path(script, src_path)
assert self._on_download_executed

@pytest.mark.asyncio
@pytest.mark.skipif(sys.version_info < (3, 8), reason="AsyncMock requires python 3.8+")
async def test_download_json(self):
expected = {"key": "val"}

storage = mock.AsyncMock()
storage.new_destination.return_value.download_bytes.return_value = json.dumps(
expected
).encode("utf-8")
src_path = "/test/path"
ctx = self._get_work_context(storage)

ctx.download_json(src_path, partial(self._on_download, expected))
steps = ctx.commit()
await steps.prepare()
await steps.post()
self._assert_src_path(steps, src_path)
script = ctx.commit()
await script._before()
await script._after()

self._assert_src_path(script, src_path)
assert self._on_download_executed

@pytest.mark.parametrize(
Expand All @@ -135,19 +140,17 @@ async def test_download_json(self):
def test_start(self, args):
ctx = self._get_work_context()
ctx.start(*args)
steps = ctx.commit()
script = ctx.commit()

c = CommandContainer()
steps.register(c)
batch = script._evaluate()

assert c.commands() == [{"start": {"args": args}}]
assert batch == [{"start": {"args": args}}]

def test_terminate(self):
ctx = self._get_work_context(None)
ctx.terminate()
steps = ctx.commit()
script = ctx.commit()

c = CommandContainer()
steps.register(c)
batch = script._evaluate()

assert c.commands() == [{"terminate": {}}]
assert batch == [{"terminate": {}}]
3 changes: 1 addition & 2 deletions yapapi/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from pkg_resources import get_distribution

from yapapi.ctx import ExecOptions, Work, WorkContext
from yapapi.engine import NoPaymentAccountError, WorkItem
from yapapi.engine import NoPaymentAccountError
from yapapi.executor import Executor, Task
from yapapi.golem import Golem

Expand Down Expand Up @@ -55,5 +55,4 @@ class _WindowsEventPolicy(asyncio.events.BaseDefaultEventLoopPolicy):
"Golem",
"NoPaymentAccountError",
"Work",
"WorkItem",
]
Loading