Decouple scripts from WorkContext #609

kmazurek · 2021-08-26T22:38:21Z

Resolves #564, #261

Public API changes

introduced a new entity (script/__init__.py#Script) which replaces direct command calls on the WorkContext.
all command methods on WorkContext have been marked as deprecated in version 0.7.0.
WorkContext now includes a new_script method which returns an instance of Script bound to the given WorkContext instance.
each call adding a new command to a Script (e.g. run, send_json or direct add call) now returns an awaitable which, once the script is executed, will contain the event with the result of that particular command.

Other changes

Script is now used as the expected yield type from worker functions.
removed the use of CommandContainer, Steps and ExecOptions classes. Their former responsibilities are now fulfilled by Script (i.e. storing commands, defining options for a batch and serializing a series of commands for transfer to the provider).
all classes representing commands (e.g. Run, SendJson) have been moved to script/command.py. They have a new common base class (Command) with a new interface (after, before, evaluate).

TODO (optional, perhaps as separate PRs)

update all existing examples and integration tests to use Script instances instead of calls to WorkContext
remove stuff deprecated in 0.6.0?
(once possible) update the API reference

yapapi/script/__init__.py

yapapi/script/command.py

johny-b

I didn't check the tests. I will do this later.

I have a general remark I think we might need to discuss. I really dislike the documentation being duplicated between yapapi.script.command.X and yapapi.script.Script.x. This is hard to maintain: docs will be changing and it's hard to keep such things in sync. Also, I double-dislike duplicated logic, like in run (I added a separate comment for this).

I think all those functions should look like this:

def run(self, *args, **kwargs):
    '''Arguments are passed to command.Run(). Returns awaitable ...'''
    return self.add(Run(*args, **kwargs))

This is clear. This is easy to maintain. If we have links in docs (as Filip suggested), this wouldn't even make it much harder to read.

Also: chapeau bas for finding a path through all of this stuff!

yapapi/script/command.py

yapapi/engine.py

yapapi/script/__init__.py

Co-authored-by: johny-b <[email protected]>

kmazurek · 2021-09-07T11:09:42Z

I have a general remark I think we might need to discuss. I really dislike the documentation being duplicated between yapapi.script.command.X and yapapi.script.Script.x. This is hard to maintain: docs will be changing and it's hard to keep such things in sync. Also, I double-dislike duplicated logic, like in run (I added a separate comment for this).

I think all those functions should look like this:
def run(self, *args, **kwargs):
    '''Arguments are passed to command.Run(). Returns awaitable ...'''
    return self.add(Run(*args, **kwargs))
This is clear. This is easy to maintain. If we have links in docs (as Filip suggested), this wouldn't even make it much harder to read.

While I agree on the part about not duplicating docstrings I'm not convinced about having these methods accept *args, **kwargs. These methods (e.g. Script.run) are intended as helpers, providing a higher level of abstraction over the more generic Script.add.
As such, I think their signature should be unambiguous so that the user immediately understands how to use a given method, relying solely on its signature without having to jump to a docstring of a different class (for example: send_file(src_path: str, dst_path: str) versus send_file(*args, **kwargs)).
Another case here is type checking the user's code e.g. with mypy. With generic arguments (*args, **kwargs) it's not going to work.
Another thing to consider here is whether these existing commands/methods are likely to change their interface in the future. From my point of view they're more or less a closed interface (e.g. all download and upload commands will probably not change their arguments).

@johny-b What's your take on this?

johny-b · 2021-09-07T14:56:52Z

Another case here is type checking the user's code e.g. with mypy. With generic arguments (*args, **kwargs) it's not going to work.

Are sure about this?
I never tried that, but I don't really understand why this wouldn't work, mypy has all the information it needs from the Command invocation (e.g. Run(...)). I think decorators usually have *args, **kwargs and they work with mypy?

Anyway, that's not really important because ...

@johny-b What's your take on this?

... I think you're right. *args, **kwargs are unhelpful for the end user, this was a bad idea.

shadeofblue · 2021-09-08T06:00:07Z

tests/script/test_script.py

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


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....

Done in 53d23eb

shadeofblue

generally, looks good as far as I could tell... adding a few non-critical comments

shadeofblue · 2021-09-08T06:00:49Z

tests/script/test_script.py

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


leftover cruft?

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.

Removed in 53d23eb

shadeofblue · 2021-09-08T09:46:28Z

tests/script/test_script.py

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


leftover cruft?

Same as above.

Removed in 53d23eb

shadeofblue · 2021-09-08T09:47:30Z

tests/test_ctx.py

-        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'}


same leftover cruft...

It's not cruft!

Removed anyway in 53d23eb

shadeofblue · 2021-09-08T09:47:35Z

tests/test_ctx.py

-        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'}


same leftover cruft...

Removed in 53d23eb

shadeofblue · 2021-09-08T09:50:35Z

yapapi/script/__init__.py

+        """Schedule a Terminate command on the provider."""
+        return self.add(Terminate())
+
+    def send_json(self, data: dict, dst_path: str) -> Awaitable[CommandExecuted]:


shouldn't this be upload_json to better correspond with download_json ?

Renamed in 4741009

shadeofblue · 2021-09-08T09:51:28Z

yapapi/script/__init__.py

+        """
+        return self.add(SendBytes(data, dst_path))
+
+    def send_file(self, src_path: str, dst_path: str) -> Awaitable[CommandExecuted]:


shouldn't this be upload_file to better correspond with download_file ?

Renamed in 4741009

shadeofblue · 2021-09-08T09:54:18Z

yapapi/ctx.py

-        steps = self._pending_steps
-        self._pending_steps = []
-        return Steps(*steps, timeout=timeout)
+        assert self.__script._commands, "commit called with no commands scheduled"


hmm.... maybe it's not a bad thing per se? ... I mean, maybe there should be a way to vary commands run within an exescript from 0 to n ? ... it would make it simpler to create scripts based on variable number of elements - the client code wouldn't have to vary here and just process_batches would step over such a script without actually sending anything to the provider...

Alright, though I'm thinking of potential debugging issues here (someone sending an empty script by mistake). Perhaps we should print a warning in process_batches whenever an empty script is encountered?

printing a warning is perfect! thanks :)

yapapi/script/__init__.py

yapapi/script/command.py

Co-authored-by: shadeofblue <[email protected]>

johny-b

Good job!

There's one more thing I don't understand, I left a commit suggestion.

Also: please take a look at
#641

yapapi/script/__init__.py

johny-b

OK, I like the result : )

kmazurek added 8 commits August 19, 2021 14:16

Create Script class, move relevant logic to its package

fef588d

WIP: add evaluation to script steps

9484671

WIP: add new_script to WorkContext

b767676

Add timeout and wait_for_results to Script init

22373aa

Make script commands a public interface

9b5010b

Enable awaiting individual commands of a script

0351664

Make Script._serialize synchronous

5f385c3

Add direct step calls to Script

efaf2c3

kmazurek self-assigned this Aug 26, 2021

Make WorkContext backward compatible with Script

6f5fcfd

kmazurek force-pushed the km/context-script-decouple branch from 95363d6 to 6f5fcfd Compare August 27, 2021 08:10

Merge branch 'master' into km/context-script-decouple

2af95d3

azawlocki reviewed Aug 27, 2021

View reviewed changes

yapapi/script/__init__.py Outdated Show resolved Hide resolved

azawlocki reviewed Aug 27, 2021

View reviewed changes

yapapi/script/__init__.py Outdated Show resolved Hide resolved

azawlocki reviewed Aug 27, 2021

View reviewed changes

yapapi/script/command.py Outdated Show resolved Hide resolved

kmazurek added 15 commits August 30, 2021 12:46

Fix isinstance check in _set_cmd_result

3742e18

Update type hint on *args

063c684

Adapt WorkContext unit tests

8278256

Add Script unit tests

6a95fc7

WIP: test agr termination debugging

3d9c123

Fix script handling in process_batches

cc27a0a

Update Script unit tests

9d7f2aa

Update types based on mypy

d8430ed

Remove unused classes

92775fc

Address flake8 errors

8214af1

Fix AsyncMock imports for Python <3.8

69150eb

Merge branch 'master' into km/context-script-decouple

f788d54

Add missing docstrings

8dfea06

Merge branch 'master' into km/context-script-decouple

3386271

Add Script docstring

f503432

kmazurek marked this pull request as ready for review September 2, 2021 10:44

kmazurek requested review from a team, johny-b and shadeofblue September 2, 2021 10:44

johny-b suggested changes Sep 2, 2021

View reviewed changes

Reverse order of stdout and stderr in Run#__init__

5378db3

Co-authored-by: johny-b <[email protected]>

kmazurek force-pushed the km/context-script-decouple branch from 7dd990d to 5378db3 Compare September 3, 2021 13:21

kmazurek added 4 commits September 3, 2021 17:54

Change Script fields to be instance variables

82d2cd7

Add id property to Script instances

4281648

Move command result future to Command field

c086011

Update type hint on Command result

e574447

shadeofblue reviewed Sep 8, 2021

View reviewed changes

kmazurek and others added 7 commits September 8, 2021 14:04

Add return statements to WorkContext command methods

fbb32b1

Add a factory for WorkContext objects in tests

53d23eb

Rename send_* methods to upload_* on Script

4741009

Make stdout and stderr optional on Script.run

37f3dfe

Remove redundant condition from Run.evaluate

64f4a17

Update yapapi/script/__init__.py

804f765

Co-authored-by: shadeofblue <[email protected]>

Merge branch 'master' into km/context-script-decouple

b9f4d32

johny-b self-requested a review September 9, 2021 10:38

johny-b reviewed Sep 9, 2021

View reviewed changes

yapapi/script/__init__.py Show resolved Hide resolved

johny-b self-requested a review September 9, 2021 13:40

johny-b approved these changes Sep 9, 2021

View reviewed changes

Merge branch 'master' into km/context-script-decouple

41d731e

kmazurek merged commit cebb56c into master Sep 9, 2021

kmazurek deleted the km/context-script-decouple branch September 13, 2021 07:56

kmazurek mentioned this pull request Sep 21, 2021

[enhancement] return an awaitable future from each WorkContext add-step command #369

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple scripts from WorkContext #609

Decouple scripts from WorkContext #609

kmazurek commented Aug 26, 2021 •

edited

Loading

johny-b left a comment •

edited

Loading

kmazurek commented Sep 7, 2021

johny-b commented Sep 7, 2021

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue left a comment

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021 •

edited

Loading

shadeofblue Sep 8, 2021

kmazurek Sep 8, 2021

shadeofblue Sep 9, 2021

johny-b left a comment •

edited

Loading

johny-b left a comment

Decouple scripts from WorkContext #609

Decouple scripts from WorkContext #609

Conversation

kmazurek commented Aug 26, 2021 • edited Loading

Public API changes

Other changes

TODO (optional, perhaps as separate PRs)

johny-b left a comment • edited Loading

Choose a reason for hiding this comment

kmazurek commented Sep 7, 2021

johny-b commented Sep 7, 2021

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

shadeofblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmazurek Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johny-b left a comment • edited Loading

Choose a reason for hiding this comment

johny-b left a comment

Choose a reason for hiding this comment

kmazurek commented Aug 26, 2021 •

edited

Loading

johny-b left a comment •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

kmazurek Sep 8, 2021 •

edited

Loading

johny-b left a comment •

edited

Loading