Implement stateless function support#42
Conversation
|
Hi @GratefulTony, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
1 similar comment
|
Hi @GratefulTony, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
|
@GratefulTony thanks a lot for this contribution :) |
sleipnir
left a comment
There was a problem hiding this comment.
@GratefulTony Thank you for this PR. Technically very good. I made just a few small comments.
@marcellanz hing to add? We have an open question about nomenclatures and semantics about StatelessFunctions in the Cloudstate main repository but I think we can go ahead with the PR analysis and then make the necessary changes. What do you think?
| TYPE_URL_PREFIX = "type.googleapis.com/" | ||
|
|
||
|
|
||
| def get_payload(command): |
There was a problem hiding this comment.
I see some duplications between this class and the CloudstateEventSourcedServicer class that opens up possibilities for refactoring. Perhaps extracting to a utility class.
| forward: Forward = None | ||
|
|
||
| # todo: is this correct? there is no command_id on the stateless function requests. | ||
| command_id = random.randint(0, sys.maxsize) |
There was a problem hiding this comment.
Yes. As there is no state there is no need for the proxy to map an entity to a specific shard. So there is no need for a command_id
| stream_out_handlers: MutableMapping[str, Callable] = field(default_factory=dict) | ||
|
|
||
| @property | ||
| def persistence_id(self): |
There was a problem hiding this comment.
I don't think we need a persistence_id because in the stateless case there will be no persistence involved
There was a problem hiding this comment.
this is the last issue I need to resolve: I haven't been able to get discovery to work without this.
|
|
||
| logger = getLogger() | ||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Since we don't have a reference TCK for StatelessFunctions yet implemented, wouldn't it be more interesting to turn this into unit testing / integration? Wdyt?
There was a problem hiding this comment.
work in progress moving to unit tests.
There was a problem hiding this comment.
looks like the proxy needs to be running to get any meaningful test of the functionality. I think the only solution would be to pull/boot the docker container during ci, which might be annoyingly slow.
There was a problem hiding this comment.
I think it's OK to have the CI pulling docker images, hence the proxy, to test functionality; we do it at different other points. If not a language support library is doing it, the main project does it when it runs the TCK for all known support libraries; which might be too late depending on the changes made.
There is also: https://observablehq.com/@marcellanz/cloudstate-go-tck-verification that runs the TCK nightly.
|
Thanks for the review! I will look into making the requested changes this weekend. Also open to updating things in the future if nomenclature/best practices around stateless functions change. |
|
Current plan is to rename stateless functions to Actions. They can certainly be renamed here later. |
|
@pvlugter Is that the only change you intend to make? |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
1 similar comment
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
|
still have some work to do implementing the requested changes. Will get to it in the coming days. Moving to running unit tests for stateless functions. (can look into renaming to actions) fixed up the tck script to use unique container names, and made the networking more platform agnostic. |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
1 similar comment
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
ef98f92 to
3641a2e
Compare
@GratefulTony I see now the typesafe-cla-validator mentioning "chomnoue" for commits you probably pushed. Also there are quite few unrelated commits I think visible in this branch. Could you rebase your changes/additions somehow cleanly to your branch? Regarding the CLA Validator if I remember correctly you got approved its check with your initial commit for this PR, or is this still pending? Thanks :) |
@pvlugter can you imagine why the CLA Validator mentions chomnoue now in this PR? Thanks. |
Yeah. I got approved, but had to go back and rewrite some of my earlier commits since I had my email misconfigured in git config on a new dev machine. I'm in the middle of a rebase to try to clean things up. this is my best guess as to why chomnoue is getting flagged. |
|
We discussed the renaming from stateful function to action today at the contributors call and we agreed to: "Python stateless function support can rename to action now or after merge". So I think after the cleanup and review of this PR we should merge this PR and get the name changed afterwards. |
|
@marcellanz I agree with merge first and rename later |
1665c76 to
f284b8d
Compare
|
I think I have a good rebase now, CLA validator appears to be happy for now. still a tiny bit of work to do to get the docker/proxy-aware unit tests working. |
Thanks @GratefulTony sounds great. With about 4k lines removed and 1.5k added, can you please summarize shortly what has added, changed (and removed also) in this PR or what you were looking for. I assume that the PRs title "Implement stateless function support" describes not all changes for what I can see. |
|
regarding why the persistence_id was included in the implementation: as I had said, it appears that the service discovery mechanism is using an EntitySpec protocol buffer to participate in service description. The EntitySpec has the entities field (of type Entity) which requires the persistence_id field. In my implementation, I do not use the persistence_id to do service resolution, so as far as I can tell, it is a kind of baggage, but a field which I think needs to be populated on the Entity protocol buffer. I don't see any corresponding analogue for stateless functions in the protobuf specs. I wasn't sure if this was used in any way by cloudstate internals, but I set it to the service name to relate the user function to the namespace concept. In my implementation, I believe it does not actually need to be set to non-default, so I could potentially set this field in |
|
It seems the proxy doesn't care ATM: |
|
There is clearly a gap between what was defined in the protobuf and what the proxy implements. But I think that to continue with this PR we can maintain the approach used by @GratefulTony just referring to the persistence id in the discovery stage |
|
@sleipnir I Agree. |
|
@sean-walsh would you like to have an eye on this community PR for the Python support? Thanks. |
|
So it seems we're ok with the "ride along" persistence_id and renaming to Actions later. I think other than that, all of the requested changes have been implemented. Let me know if there are other changes requested, otherwise, pr should be about ready to merge. |
|
Thank you @GratefulTony I will take a look later. Again welcome to our community |
Co-authored-by: Dalmo Cirne <dalmo.cirne@gmail.com>
Co-authored-by: Dalmo Cirne <dalmo.cirne@gmail.com>
|
I tested the PR locally (on a Mac) and |
| PROXY_NAME=cloudstate-proxy-$RUN_SUFFIX | ||
| USER_FUNCTION_NAME=cloudstate-function-$RUN_SUFFIX | ||
| FUNCTION_CLIENT_NAME=cloudstate-function-client-$RUN_SUFFIX | ||
| TCK_NAME=cloudstate-tck-$RUN_SUFFIX |
There was a problem hiding this comment.
| TCK_NAME=cloudstate-tck-$RUN_SUFFIX | |
| TCK_NAME=cloudstate-tck-$RUN_SUFFIX | |
| PYTHON_TCK_NAME=cloudstate-python-tck-dev:$RUN_SUFFIX |
| @@ -0,0 +1,66 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
after running this script I have a stopped container and its image left on the box
dev-cloudstate-tck:local
I think we can at least remove the container.
| NETWORK_NAME=tck-network-$RUN_SUFFIX | ||
|
|
||
| # fresh docker build | ||
| docker build -t dev-cloudstate-tck:local ./ |
There was a problem hiding this comment.
| docker build -t dev-cloudstate-tck:local ./ | |
| docker build -t $PYTHON_TCK_NAME ./ |
| -e USER_FUNCTION_PORT=8090 \ | ||
| cloudstateio/cloudstate-proxy-dev-mode | ||
| sleep 10 | ||
| docker run -d --network $NETWORK_NAME --name $USER_FUNCTION_NAME -p 8080:8080 dev-cloudstate-tck:local \ |
There was a problem hiding this comment.
| docker run -d --network $NETWORK_NAME --name $USER_FUNCTION_NAME -p 8080:8080 dev-cloudstate-tck:local \ | |
| docker run -d --network $NETWORK_NAME --name $USER_FUNCTION_NAME -p 8080:8080 $PYTHON_TCK_NAME \ |
| docker rm -f $USER_FUNCTION_NAME | ||
|
|
||
| # secondary integration tests for stateless function: | ||
| docker run -d --network $NETWORK_NAME --name $USER_FUNCTION_NAME -p 8080:8080 dev-cloudstate-tck:local \ |
There was a problem hiding this comment.
| docker run -d --network $NETWORK_NAME --name $USER_FUNCTION_NAME -p 8080:8080 dev-cloudstate-tck:local \ | |
| docker run -d --network $NETWORK_NAME --name $USER_FUNCTION_NAME -p 8080:8080 $PYTHON_TCK_NAME \ |
| -e USER_FUNCTION_PORT=8080 \ | ||
| cloudstateio/cloudstate-proxy-dev-mode | ||
| sleep 10 | ||
| docker run --network $NETWORK_NAME --name $FUNCTION_CLIENT_NAME dev-cloudstate-tck:local \ |
There was a problem hiding this comment.
| docker run --network $NETWORK_NAME --name $FUNCTION_CLIENT_NAME dev-cloudstate-tck:local \ | |
| docker run --network $NETWORK_NAME --name $FUNCTION_CLIENT_NAME $PYTHON_TCK_NAME \ |
|
|
||
| status1=$? | ||
|
|
||
| docker rm -f $PROXY_NAME |
There was a problem hiding this comment.
the existing tck scripts have the advantage to clean up every container/image no matter what happenend through a TRAP:
finally() {
docker rm -f "$PROXY"
docker rm -f "$FUNC"
}
trap finally EXIT
https://github.com/cloudstateio/python-support/blob/master/tck/run_tck.sh
| status1=$? | ||
|
|
||
| docker rm -f $PROXY_NAME | ||
| docker rm -f $USER_FUNCTION_NAME |
There was a problem hiding this comment.
I think we should remove the temporary python tck image too:
| docker rm -f $USER_FUNCTION_NAME | |
| docker rm -f $USER_FUNCTION_NAME | |
| docker rmi $PYTHON_TCK_NAME |
|
thanks @marcellanz The changes to the script should be easy. Regarding the TCK, I noticed things weren't working as well when a new version of the TCK container was pushed a few days ago. I will try to get to the bottom of what's going on with the TCK. |
@GratefulTony It would be interesting from where the I made my comment more about this one: which seems to come from the new stateless function tests. Thanks for looking into :) |
|
@marcellanz I see. Thanks for pointing that out. Interesting that you don't appear to be using the latest tck image, which iirc, runs 23 tests? I may also need to check that the tck script pulls the latest image version... It also seemed that there might be an issue with the latest tck... is this why you were using the previous version? |
great point! I think I have still an old image on my box. |
|
I think we can only consider the oldest tck to validate this PR. |
@sleipnir Agree. The last few days too much has changed on cloudstate master. We could use @GratefulTony the "old" tck script had also the options to specify alternative docker images used: |
…tefulTony/python-support into implement-stateless-function-support
|
Requested changes made to TCK shell script. Also pinned TCK image version as suggested. TCK passes for me on mac and linux. I tested on two separate macs with clean clones of the repo and wasn't able to reproduce the failed test case. |
|
I ran on my linux machine and saw no problems. |
|
@GratefulTony thanks again for your contribution and efforts on this PR :) |
|
Thanks everyone. Excited for the future of this project. |
|
🎉 👍 |
This implements stateless function support. A provisional stateless function TCK is included since to my knowledge, the cloudstate tck does not cover stateless functions yet. By default, executing ./extended_tck.sh will build a docker container, install
cloudstate python-supportinto the docker container (including protobuf build), and run the original tck as well as the extended tck against said container.Some changes to the build have been made to simplify installation, incorporating protobuf compilation into setup.py.