Closed yaacov closed 6 months ago
yaacov
commented
10 months ago @nirs can you take a look ? @bennypowers hi, can you take a look at the js parts ? @slaviered hi, this should not change the docs and classroom documentation, if it does, I will fix them.
nirs
commented
10 months ago Cool demo!
But about 20x times too big to review. Can you break up to small easy to review commits?
In general the idea that the server connects to the drivers looks strange and harder to use.
The current model when the clients connects to the server is better. I don't see why we need to change it.
Regarding the problems:
a. We can not create a k8s Pod for each driver, and let them run forever. When a driver starts it immediately try to connect to the server and blocks other drivers from running.
Why do you want to run the client forever? The client is something you run temporarily when testing or during a game.
b. We use python RPC protocol to comunicate between the driver and the server, this makes it hard to write drivers in compiled languages, making us relay on .pyc file to hide implementation code.
This is incorrect. We use simple line based JSON lines RPC protocol that is easy to write any any languages and is not related to python in any way.
The protocol for for communicating with the server is not related to the the driver pyc file in any way. The dependency is the python client which can be replaced with a client written in any other language.
c. We use 2.7 oriented libraries: twisted, autobahn and pytest.
These libraries are not python 2.7 oriented, they just mature libraries that were popular years before python 3 was started.
Twisted is awesome because it let you run multiple services clients in the same process. I don't think there is anything replacing it. For example, if you want to add an IRC clinet reporting game results to IRC channel, this is really easy in twisted.
autobahn is web socket library with twisted integartion. websoket is the best way to commmunicate with the game UI, allowing real time updates. Why replace it?
pytest is the best testing library for python (if not the best testing library for anything). I'm not sure what is the issue with it.
Since we used twisted, it was very easy to add xmlrpc server for the admin tool, but we should realy get rid of the xmlrpc code since simple http (already used by the UI) is good enough for the admin tool, or maybe we can just drop the admin tool - since the web UI was added, there is no need for the admin tool.
It will be more productive to open issues for any problem and discuss them.
yaacov
commented
10 months ago @nirs thanks
This PR is to make the Game more Openshift ready, when I run pods in Openshift, I want to deploy my pods, and let them run.
Why do you want to run the client forever? The client is something you run temporarily when testing or during a game.
"As an instructor I want to ask the students to deploy their drivers to the cluster, and then run matches using an already running server and drivers"
removing the xmlrpc connection between the driver and server makes it easy to run the game-engine (named "server") and the driver (named "client") as Pods that live as good citizens of the cluster.
you need to run the server after the clients
you don't need to re-run the server, the idea is that the server is running all the time. you only need to choose the drivers for the next match, and you do it while the server is running.
you need to know the clients address to run the server
this is easy when running on a cluster:
oc get services -n studentsyou need to restart the server for every game
the server is good citizen of the cluster, it is deployed once and run forever, the only caveat is that the game engine "server" has a state while running a game, so you can only have one "server" serving a set of games.
you can run several game-engines "servers" but they will need to have a URL per server and can't run using a load balancer, the drivers on the other hand are stateless and can run in deployments of more then one.
the server need to handle client disconnection - previously the client could reconnect
the game-engine (named "server" because it is currently called this way) is a client or the driver (named "client" because it is currently called this way) server, and does not care if the driver is connected or not, it just pool it, it can get an answer of miss it, nothing will happen if no answer is given except the the car will continue forward and the user will get a notice.
yaacov
commented
10 months ago This is incorrect. We use simple line based JSON lines RPC protocol that is easy to write any any languages and is not related to python in any way.
The protocol for for communicating with the server is not related to the the driver pyc file in any way. The dependency is the python client which can be replaced with a client written in any other language.
@nirs ok, so it uses twisted protocol instead of xmlrpc ... https://twisted.org/documents/8.1.0/api/twisted.protocols.basic.LineReceiver.html
not sure if it's that easy to implement in other languages, but I didn't try :-) I still think that simple HTTP client server is more clean and clear, and easy to implement in "any language" (tm)
yaacov
commented
10 months ago Can you break up to small easy to review commits?
Yes, I will see how to make it more clear :+1:
yaacov
commented
10 months ago Twisted vs asyncio
asyncio is a part of the standard library. no need to install it, it's a nice plus for using it. asyncio: Uses async/await syntax, this is debatable, but I think that most users like it, this pattern is very common in other langauges too, so it's also a plus. asyncio: works nice with aiohttp, while twisted works nice with autobahn ... so it's a tie here :-)
To sum things up, Twisted is awesome, but asyncio is integrated with python and is "the new cool thing" for me it's 2 plus for asyncio and one tie :-)
yaacov
commented
10 months ago pytest vs unittest
unittest: Part of the Python standard library. You don't need to install anything extra to use it. unittest: Doesn't support parameterized testing out of the box. You'd need to use additional tools or libraries
so it's one plus for unittest and one for pytest :-) but the fact it's part of Python makes me think it's nicer to use.
nirs
commented
10 months ago Twisted vs asyncio
asyncio does not replace twisted - it is just the core of twisted, and I think that twisted can use the asyncio event loop if you want.
I don't see how this is related to making the containers more friendly.
pytest vs unittest
Testing with pytest is so much better than anything else and it worth the tiny effort
of doing pip install pytest.
I cannot think about more unrelated topic to making the container more friendly :-)
Let focus on the friendliness issue, because this seem to the purpose of this PR. Please open an issue about the current containers and explain how the current containers are used today and what are the issues that students or instructors have with the current containers.
yaacov
commented
10 months ago asyncio does not replace twisted - it is just the core of twisted, and I think that twisted can use the asyncio event loop if you want.
For basic HTTP client server communication, you don't need twisted, asyncio is doing all that is needed. Anything fancier that require twisted will make it harder to implement using tools and languages that don't have twisted.
I cannot think about more unrelated topic to making the container more friendly :-)
ok :-)
yaacov
commented
10 months ago @nirs hi,
a. reverted ci to use pytest, still need to translate the tests. b. started splitting the PR, now it's split between metadata commits and code commits, still need to split the code commits further.
yaacov
commented
6 months ago Closing in favor of specific repository per game container:
| component | repo |
|---|---|
| Game engine | https://github.com/RedHat-Israel/rose-game-engine |
| Game web based user interface | https://github.com/RedHat-Israel/rose-game-web-ui |
| Game car driving module | https://github.com/RedHat-Israel/rose-game-ai |
I also added a reference container for running machine learning driver: https://github.com/RedHat-Israel/rose-game-ai-reference
to run on all components on a cluster:
kubectl apply -f https://raw.githubusercontent.com/RedHat-Israel/rose-game-engine/main/rose-game.yaml
to run locally:
podman run --rm --network host -it quay.io/rose/rose-game-engine:latest
podman run --rm --network host -it quay.io/rose/rose-game-web-ui:latestand for the driver, create mydriver.py file locally, and run:
podman run --rm --network host -it \
-v $(pwd)/mydriver.py:/mydriver.py:z \
-e DRIVER /mydriver.py \
quay.io/rose/rose-game-ai:latest
TL;DR demo
Screencast from 2023-09-11 16-09-37.webm
Ref: Python machine learning driver: https://github.com/yaacov/rose-ml-driver Go land driver: https://github.com/yaacov/rose-go-driver
What this POC try to solve:
a. We can not create a k8s Pod for each driver, and let them run forever. When a driver starts it immediately try to connect to the server and blocks other drivers from running. b. We use python RPC protocol to comunicate between the driver and the server, this makes it hard to write drivers in compiled languages, making us relay on .pyc file to hide implementation code. c. We use 2.7 oriented libraries: twisted, autobahn and pytest.
Fix a and b: Replace the RPC calls with "vanilla" client server calls, the server can now connect to selected drivers while the other drivers still running. Fix c: Replace the 2.7 oriented libraries with 3.x native libraries, websockets and aiohttp.
What is changing:
The admin needs to actively select the drivers to run the match instead of waiting for two students to connect. All students must run there drivers before the match begin.
Running locally: