Signals and ATP v2 #98

jaredoconnell · 2023-08-25T23:17:45Z

Changes introduced with this PR

This PR adds signals to the schema types, and adds support for ATP v2.

For backwards compatibility, the old format of the step decorator is available for plugins that don't require signals.
For plugins that require signals, or for steps that benefit from their own object, you use @step_with_signals instead of @step, and you pass in two items. The first is the constructor (or a lambda that calls the constructor) for the object that has the step and signal methods, and the second is the function names for the signals. The signals are then processed later.

I needed to make some compromises to workaround Python's limitations.
For passing in associated signals with steps, I needed to pass in method names then load the methods later instead of passing in references to the methods, because the decorator is called before the class is fully defined.

By contributing to this repository, I agree to the contribution guidelines.

This should be backwards-compatible

Also moved ATP server into a class

Also fixed a few enum issues

Also refactored to improve the code

Also did a mild refactor of function

src/arcaflow_plugin_sdk/atp.py

src/arcaflow_plugin_sdk/test_atp.py

src/arcaflow_plugin_sdk/atp.py

jdowni000 · 2023-09-19T18:36:12Z

This looks great to me. Let's handle the things Matt found. Great work.

This appears to be causing a failure without any reason why

webbnh

Now that this is merged, I can comment with impunity (especially since I'm about to go on PTO and won't be able to resolve the "conversations"...). 😆

I didn't actually finish the review, but I'm out of time, so here's what I got.

Most of my comments are trivialities which you can take for what they are worth, but I did find a couple of things worth looking closely at:

There are a couple of places where you are flushing stdin which seems potentially problematic.
There is a place where you're doing a CV wait which looks improperly structured (although, it's possible that it doesn't matter as much in Python as it does in a "real language" 😉).

Other items of somewhat lesser note:

Catching SystemExit is a questionable thing to do. With your changes, it doesn't look like it's needed any more, but, if it is, it should be closely examined to see if it's the "right" mechanism.
There's some I/O redirection which looks weird, to me (but, maybe it's just one more "special thing" that Python can do...).
The error handling for CBORDecodeError seems inconsistent.

Cheers!

webbnh · 2023-09-20T20:10:29Z

src/arcaflow_plugin_sdk/atp.py

-import signal
 import sys
 import typing
+import threading
+import signal


Do you keep your import lists ordered? (E.g., you might want to try out isort.)

Should the import of enum be in this block?

webbnh · 2023-09-21T16:47:13Z

src/arcaflow_plugin_sdk/atp.py

+    WORK_DONE = 1
+    SIGNAL = 2
+    CLIENT_DONE = 3


Consider using enum.auto() instead of explicit values.

Since we need to sync these between the go and Python SDK, I think it makes sense to make these explicit.

webbnh · 2023-09-21T16:55:18Z

src/arcaflow_plugin_sdk/atp.py

+        """
+        signal.signal(signal.SIGINT, signal_handler)  # Ignore sigint. Only care about arcaflow signals.


Why use a custom signal handler (i.e., signal_handler) instead of using the standard signal.SIG_IGN to ignore the signal?

Good point. I'll need to refactor this at some point.

webbnh · 2023-09-21T16:56:18Z

src/arcaflow_plugin_sdk/atp.py

+        if os.isatty(self.stdout.fileno()):
+            print("Cannot run plugin in ATP mode on an interactive terminal.")
+            return 1


This is curious...why is this the case? Do/should you have similar restrictions on stderr or stdin?

The plugins communicate over stdout and stdin

Understood, but does that mean that there should never be a terminal connected there (not even for interactive testing)? (Is it even possible that this could happen?) That is, is there actually value to performing this check?

If so, then shouldn't you be checking stdin as well?

webbnh · 2023-09-21T17:07:27Z

src/arcaflow_plugin_sdk/atp.py

-        if message["config"] is None:
-            stderr.write("Work start message is missing the 'config' field.")
+            # Serialize then send HelloMessage
+            start_hello_message = HelloMessage(2, plugin_schema)


The 2 here looks like a "magic number"...shouldn't that be a symbolic reference to an externally-defined constant? Or, perhaps, the whole HelloMessage(2, plugin_schema) expression should be encapsulated in a reference?

I should create a version constant.

webbnh · 2023-09-21T19:23:44Z

src/arcaflow_plugin_sdk/schema.py

+    def __call__(
+        self,
+        step_data: StepObjectT,
+        params: SignalDataT,
+    ):
+        """
+        :param params: Input data parameter for the signal handler.
+        """


Since the docstring is describing params, shouldn't it describe step_data, as well?

webbnh · 2023-09-21T19:37:05Z

src/arcaflow_plugin_sdk/schema.py

+        # Create a map to populate
+        signal_handlers_map = {}
+        for handler_method_name in self.signal_handler_method_names:
+            # Retrieve the object attributes, which will be the schemas
+            handler = getattr(object_instance, handler_method_name)
+            signal_handlers_map[handler.id] = handler
+        self.signal_handlers = signal_handlers_map


Consider using a dictionary comprehension:

self.signal_handlers = { handler.id: getattr(object_instance, handler_method_name) for handler_method_name in self.signal_handler_method_names }

webbnh · 2023-09-21T19:56:06Z

src/arcaflow_plugin_sdk/schema.py

+    def get_step(self, step_id: str):
+        if step_id not in self.steps:
+            raise NoSuchStepException(step_id)
+        return self.steps[step_id]


If step_id not being in self.steps is an "exceptional" condition, consider coding it as such:

def get_step(self, step_id: str): try: return self.steps[step_id] except KeyError as e: raise NoSuchStepException(e.args[0]) from None

Alternatively, avoid doing the lookup twice:

def get_step(self, step_id: str): v = self.steps.get(step_id) if v is None: raise NoSuchStepException(step_id) return v

Similarly for get_signal().

webbnh · 2023-09-21T20:08:27Z

src/arcaflow_plugin_sdk/schema.py

+        if not step.object_data_ready:
+            with step.object_cv:
+                # wait to be notified of it being ready. Test this by adding a sleep before the step call.
+                step.object_cv.wait()


This code looks questionable: it probably should hold the object_cv lock when checking the object_data_ready value, and it should be prepared for spurious wake-ups. So, something more like this would be appropriate:

with step.object_cv: while not step.object_data_ready: step.object_cv.wait()

(I would omit the code comment: it's obvious that we are waiting, it's clear what we are waiting for, and comments on how to test this belong in the test module not here....)

webbnh · 2023-09-21T20:11:01Z

src/arcaflow_plugin_sdk/predefined_schemas.py

+@dataclass
+class cancelInput:
+    pass


Why make this a @dataclass? It has no fields....

It's the practice for all classes that we're serializing/unserializing.

jaredoconnell added 30 commits August 23, 2023 18:12

Progress towards signals in SDK

9d1d770

This should be backwards-compatible

Added signals to ATP server

f952174

Also moved ATP server into a class

Merge branch 'main' into signals

2e07076

Add ATP v2 support to client

82ab335

Also fixed a few enum issues

Fix linting errors

00d0a65

Fix linting error

be01c85

Ignore linting issue

1849d09

Fix linting error

4b2561a

Fix errors introduced while trying to fix linting errors

13cc3b3

Fix missing params in test

317986f

Update dependencies

c7f0155

Update pyyaml

b692b72

Fixed typo

7b86c6b

Fix predefined schema

91226f9

Bypass class limitations by passing method names instead

0edc6e5

Delay signal retrieval

f599ad3

Added missing field, and added extra None check

a14a04b

Fix parameter passed into test case

ca012ed

Fix missing input to server read loop

21e59bf

Fix wrong signal ID

9113031

Also refactored to improve the code

Fix extra parameter passed into function

28d95e2

Fix missing parameter passed into function

a20b61e

Fix missing deserialization step

7169c78

Fix missed function rename, and fix linting err

7ab7b20

Reduce redundancy, and fix wrong var passed

68d0ecd

Added client done message, and signal atp tests

cce45b7

Fix linting errors

7cf6628

Remove join and add flush

b68efc1

Also did a mild refactor of function

Change when read thread is launched

b13a14b

Ignore sigint and manage stdout correctly

df69b67

Fix linting errors, and added comments

cc2960e

jaredoconnell marked this pull request as ready for review September 18, 2023 16:03

jaredoconnell requested review from jdowni000 and mfleader September 18, 2023 16:03

mfleader reviewed Sep 19, 2023

View reviewed changes

src/arcaflow_plugin_sdk/atp.py Show resolved Hide resolved

src/arcaflow_plugin_sdk/test_atp.py Show resolved Hide resolved

src/arcaflow_plugin_sdk/atp.py Show resolved Hide resolved

jaredoconnell added 3 commits September 19, 2023 17:14

Fix ordering problem with steps and signals

c7d9658

Added coverage config file

c79cef8

Remove unused import

3f467d4

jaredoconnell requested a review from mfleader September 19, 2023 21:18

jaredoconnell added 2 commits September 19, 2023 17:49

Removed print statements, and added fail when debug logs aren't empty

ced8b2b

Remove coverage config

6a47aae

This appears to be causing a failure without any reason why

mfleader approved these changes Sep 21, 2023

View reviewed changes

jaredoconnell merged commit 44b451e into main Sep 21, 2023
4 checks passed

jaredoconnell deleted the signals branch September 21, 2023 16:15

webbnh reviewed Sep 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signals and ATP v2 #98

Signals and ATP v2 #98

jaredoconnell commented Aug 25, 2023 •

edited

Loading

jdowni000 commented Sep 19, 2023

webbnh left a comment

webbnh Sep 20, 2023

webbnh Sep 21, 2023

jaredoconnell Sep 21, 2023

webbnh Sep 21, 2023

jaredoconnell Sep 21, 2023

webbnh Sep 21, 2023

jaredoconnell Sep 21, 2023

webbnh Sep 21, 2023

webbnh Sep 21, 2023

jaredoconnell Sep 21, 2023

webbnh Sep 21, 2023

webbnh Sep 21, 2023

webbnh Sep 21, 2023

webbnh Sep 21, 2023

webbnh Sep 21, 2023

jaredoconnell Sep 21, 2023

		"""
		signal.signal(signal.SIGINT, signal_handler) # Ignore sigint. Only care about arcaflow signals.

Signals and ATP v2 #98

Signals and ATP v2 #98

Conversation

jaredoconnell commented Aug 25, 2023 • edited Loading

Changes introduced with this PR

jdowni000 commented Sep 19, 2023

webbnh left a comment

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

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

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

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

Choose a reason for hiding this comment

jaredoconnell commented Aug 25, 2023 •

edited

Loading