Adds support for Actor Reentrancy by berndverst · Pull Request #219 · dapr/python-sdk

berndverst · 2021-05-14T02:01:43Z

Description

Adds support for Actor Reentrancy

Issue reference

#214
dapr/dapr#2901

Please reference the issue this PR will close: #214

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

berndverst · 2021-05-14T02:02:41Z

Hold off on reviewing. Will add some tests. Also it appears an existing GRPC test (which should be unrelated) is failing.

wcs1only · 2021-05-14T17:03:20Z

Hold off on reviewing. Will add some tests. Also it appears an existing GRPC test (which should be unrelated) is failing.

Forgive me if you know this already: You can actually denote a PR as a 'draft' to indicate that it isn't ready to be reviewed/merged. When you go to create a new PR, there's a little dropdown on the right side of the submit button that will let you 'Create as draft'

codecov · 2021-05-14T18:19:15Z

Codecov Report

Merging #219 (3984169) into master (5e6c6f5) will decrease coverage by 0.15%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
- Coverage   88.68%   88.53%   -0.16%     
==========================================
  Files          49       52       +3     
  Lines        1954     2068     +114     
==========================================
+ Hits         1733     1831      +98     
- Misses        221      237      +16

Impacted Files	Coverage Δ
dapr/actor/client/proxy.py	`88.57% <ø> (ø)`
dapr/clients/http/dapr_actor_http_client.py	`63.15% <85.71%> (+16.09%)`	⬆️
dapr/actor/runtime/config.py	`100.00% <100.00%> (+7.14%)`	⬆️
dapr/actor/runtime/reentrancy_context.py	`100.00% <100.00%> (ø)`
dapr/actor/runtime/runtime.py	`90.76% <100.00%> (+0.44%)`	⬆️
ext/dapr-ext-fastapi/dapr/ext/fastapi/actor.py	`54.76% <100.00%> (+25.49%)`	⬆️
ext/flask_dapr/flask_dapr/actor.py	`52.27% <100.00%> (ø)`
ext/flask_dapr/flask_dapr/__init__.py	`100.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6c6f5...3984169. Read the comment docs.

berndverst · 2021-05-18T00:05:32Z

@wcs1only I think this is ready for review now.

By the way, how exactly are the samples run? Do you think I should add a sample to this PR or do that in a separate PR?

berndverst · 2021-06-16T02:20:57Z

@halspang can you verify this looks good as far as the actors functionality goes (the right things are sent in the right spots)

halspang

Overall looks good. Just a few comments. For the header/reentrancy itself I think we're good aside from the config parsing.

halspang · 2021-06-17T13:03:55Z

+class ActorReentrancyConfig:
+    def __init__(
+            self,
+            enabled: Optional[bool] = False,


While the reentrancy config itself is optional, this isn't an optional field in the config. How does this affect the actual serialization?

This does the right thing because of the default value, but the Optional qualifier is unnecessary. I'll remove this.

halspang · 2021-06-17T13:10:30Z

+        # import to avoid circular dependency
+        from dapr.actor.runtime.reentrancy_context import reentrancy_ctx


Can you explain how this makes a circular dependency?

I honestly can't. When the import was at the module / library level all kinds of tests failed because seemingly unrelated things could not import (or a version was imported that was missing things). Doing this import here fixed the issue. The same thing is done in other places in the Python SDK. Basically what is described here: https://stackabuse.com/python-circular-imports

IIRC, the actual circular dependency was on dapr.actor.runtime which in turn depends on the http client.

halspang · 2021-06-17T13:11:26Z

+        headers: Dict[str, Union[bytes, str]] = (
+            {DAPR_REENTRANCY_ID_HEADER: reentrancy_id} if reentrancy_id else {})


Can this erase other headers that were already set/needed? Or is this the only header? I'm thinking of things like content-type or length.

Headers weren't used before and passing of custom headers hasn't been supported with Actors in Python. The invoke_method only supports a single payload that is byte-encoded. This also only returns bytes.

The behavior prior to this PR is as follows:
Invoke_method always sends an empty dictionary for the headers via send_bytes(). Then that function checks whether the Content Type and DAPR API Token headers are presented (which they aren't) and if not uses defaults. See the send_bytes method definition and the test modification I made.

This introduces no breaking change.

halspang · 2021-06-17T13:19:05Z

+        response = _run(ActorRuntime.dispatch(
+            FakeMultiInterfacesActor.__name__, 'test-id',
+            "ReentrantMethod", test_request_body, reentrancy_id=reentrancy_id))


To confirm, this is just validating that we can call a reentrant actor normally right? Not actually testing the reentrant portion?

This tests the dispatcher portion of the reentrant call normally. A similar test exists for non-reentrant calls among the existing tests.

halspang · 2021-06-17T13:25:45Z

+        async def run_parallel_actors():
+            slow = dispatchReentrantCall(
+                FakeSlowReentrantActor.__name__, "ReentrantMethod", slow_reentrancy_id)
+            normal = dispatchReentrantCall(
+                FakeReentrantActor.__name__, "ReentrantMethod", normal_reentrancy_id)


Should this be the passthrough method? That's the only time the reentrant ID is fetched from the ContextVar.

I don't understand this comment. Could you explain @halspang?

This is just a bunch of Python weirdness to run two async methods truly in parallel and gather the results to force the interleaving of the processing. It is a sanity check to make sure the ContextVars truly work as we expected, that is, ContextVars are unique to each AsyncTask and are always copied forward into nested AsyncTasks.

I understand this looks weird - @wcs1only and I paired on this.

I'm fine with the python itself, my comment was more on how we'd handle the ContextVar in the code. Right now, this path only exercises the setting of the reentrant ID. If we use the passthrough method, it should also have to fetch the reentrant ID from the ContextVar. That's the path I'd like to check with interleaving.

@berndverst what do you think about my previous comment?

berndverst · 2021-06-18T01:12:20Z

Thanks for the review @halspang. Made a small change (removing the Optional qualifier which wasn't effective anyway). Otherwise take a look at my reply to your comments. Hope it all makes sense :)

halspang

Spoke over a call about my last comment and I think what I'm looking for is more of an IT or auto-example. Once we have a good idea on what to make the actor do, we can add that in but we don't need to in this PR.

wcs1only

Looks good. +1 for adding an example that exercises this.

wcs1only · 2021-06-22T22:29:36Z

+        # import to avoid circular dependency
+        from dapr.actor.runtime.reentrancy_context import reentrancy_ctx


IIRC, the actual circular dependency was on dapr.actor.runtime which in turn depends on the http client.

berndverst force-pushed the master branch from bbe338c to 84687aa Compare May 14, 2021 02:13

berndverst commented May 18, 2021

View reviewed changes

Comment thread dapr/clients/http/dapr_actor_http_client.py

berndverst requested a review from wcs1only May 19, 2021 01:16

wcs1only requested a review from halspang May 19, 2021 01:34

berndverst assigned wcs1only Jun 16, 2021

halspang reviewed Jun 17, 2021

View reviewed changes

berndverst added 15 commits June 17, 2021 18:00

Adds reentrancy options to config

f2accfa

Receive and pass through reentrancy context

c7582fe

Use absolute import

ba8e83a

fix import

783dc30

Ensure backwards compatibility

10f37a3

Some style fixes

ff7d192

fix type errors

b6e100b

avoid circular dependency

7cbb787

Add tests for actor reentrancy

db84583

Additional tests

b6694b8

one more test

ae60220

test clean up

ce2670e

reentrancy limit var name change

7d3ce27

Move reentrancy config

3d8cf21

Not everything is optional

3984169

berndverst force-pushed the master branch from 51ca101 to 3984169 Compare June 18, 2021 01:00

berndverst requested a review from halspang June 18, 2021 01:11

halspang approved these changes Jun 22, 2021

View reviewed changes

wcs1only approved these changes Jun 22, 2021

View reviewed changes

wcs1only merged commit 61f2a8e into dapr:master Jun 24, 2021

berndverst mentioned this pull request Jul 16, 2021

Add state manager context #240

Merged

3 tasks

berndverst mentioned this pull request Oct 12, 2021

Request to make @berndverst approver of python-sdk dapr/community#98

Closed

		# import to avoid circular dependency
		from dapr.actor.runtime.reentrancy_context import reentrancy_ctx

		headers: Dict[str, Union[bytes, str]] = (
		{DAPR_REENTRANCY_ID_HEADER: reentrancy_id} if reentrancy_id else {})

Conversation

berndverst commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

berndverst commented May 14, 2021

Uh oh!

wcs1only commented May 14, 2021

Uh oh!

codecov Bot commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

berndverst commented May 18, 2021

Uh oh!

Uh oh!

berndverst commented Jun 16, 2021

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berndverst Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berndverst Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berndverst Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berndverst commented Jun 18, 2021

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

wcs1only left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

berndverst commented May 14, 2021 •

edited

Loading

codecov Bot commented May 14, 2021 •

edited

Loading

berndverst Jun 18, 2021 •

edited

Loading

berndverst Jun 18, 2021 •

edited

Loading

berndverst Jun 18, 2021 •

edited

Loading