Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ROSE containers more friendly #485

Closed
wants to merge 17 commits into from

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Sep 11, 2023

TL;DR demo

# Start student A driver container
podman run --rm --network host -it quay.io/rose/rose-ml-driver:latest --port 8081

# Start student B driver container
podman run --rm --network host -it quay.io/rose/rose-go-driver:latest --port 8082

# Start server container, and connect to the students drivers
podman run --rm --network host -it quay.io/rose/rose-server:latest --drivers \
     http://127.0.0.1:8081 http://127.0.0.1:8082

# Open browser on local machine at: http://127.0.0.1:8880
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:

# Run the client using an example driver (default port 8081, use --port to change)
./rose-client --driver examples/random-driver.py

# On a different terminal (tmux session)
./rose-server

# Connect the server to the client
./rose-admin http://127.0.0.1:8880 set-drivers http://127.0.0.1:8081

# Start a match
./rose-admin http://127.0.0.1:8880 start

@yaacov
Copy link
Member Author

yaacov commented Sep 11, 2023

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

@@ -9,11 +9,20 @@

<body>
<div id="content">
<a href="settings.html" id="settings-link">Settings</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might want to make this settings/ and rename settings.html to settings/index.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to '/settings/index.html'
note that the python server we are using can't automatically show index.html files when the request is / it show 404

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennypowers i currently put the web component in settings-form file in the same dir, is there a convention for how to name web components ? where to put tehm ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, name the module the same as the tag name

<a href="settings.html" id="settings-link">Settings</a>
<a href="#" id="info-btn">Info</a>

<div id="info-panel" class="hidden">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class=hidden vs hidden vs use a pre-built custom-element with aria announce stuff ready to go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARIA is a little out of scope here :-)
we may implement when we move to web components

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While aria is not necessary, the experience must be available to screen reader users

<button id="stop" disabled>Stop</button>
<button id="music_ctl">Music</button>
<button id="reset">Reset</button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<button id="reset">Reset</button>
<button id="reset" type="reset">Reset</button>

@@ -0,0 +1,84 @@
body.dark-theme {
background-color: #2c3e50;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider using red hat design tokens

https://ux.redhat.com/tokens

rose/server/web/settings.html Outdated Show resolved Hide resolved
rose/server/web/settings.html Outdated Show resolved Hide resolved
document.addEventListener('DOMContentLoaded', loadDrivers);
</script>
</head>
<body class="dark-theme">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use prefers-color-scheme

rose/server/web/settings.html Outdated Show resolved Hide resolved
<input type="text" id="driver2" name="driver2" class="input-field">
</div>
<div class="form-group">
<button type="submit" class="submit-button">Send</button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need the type here. or really the class

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a type to prevent form event trigger, can I avoid it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, sorry, it now a regular button ... this one do need the type :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you need to prevent default, do it in the custom element class, otherwise havea route on the http server that accepts the http formdata, for progressive enhancement

rose/server/web/settings.html Outdated Show resolved Hide resolved
@yaacov yaacov force-pushed the poc-separate-client-server branch 3 times, most recently from 0e6c0a4 to ee55c43 Compare September 11, 2023 15:35
@nirs
Copy link
Member

nirs commented Sep 11, 2023

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.

  • you need to run the server after the clients
  • you need to know the clients address to run the server
  • you need to restart the server for every game
  • the server need to handle client disconnection - previously the client could reconnect

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
Copy link
Member Author

yaacov commented Sep 11, 2023

@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 students

you 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
Copy link
Member Author

yaacov commented Sep 11, 2023

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
Copy link
Member Author

yaacov commented Sep 11, 2023

Can you break up to small easy to review commits?

Yes, I will see how to make it more clear 👍

@yaacov
Copy link
Member Author

yaacov commented Sep 11, 2023

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
Copy link
Member Author

yaacov commented Sep 11, 2023

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
Copy link
Member

nirs commented Sep 11, 2023

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
Copy link
Member Author

yaacov commented Sep 11, 2023

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 yaacov force-pushed the poc-separate-client-server branch 2 times, most recently from 22e309d to 1b64380 Compare September 11, 2023 18:03
@yaacov
Copy link
Member Author

yaacov commented Sep 11, 2023

@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 yaacov force-pushed the poc-separate-client-server branch 2 times, most recently from f64802d to 23670ca Compare September 11, 2023 21:51
@bennypowers bennypowers changed the title Make ROSE containers more freindly Make ROSE containers more friendly Sep 12, 2023
@yaacov yaacov force-pushed the poc-separate-client-server branch 4 times, most recently from 53afd6b to e2c8463 Compare September 12, 2023 10:36
  - update readme
  - move pipenv to the docs package
  - update how to check exercises doc

Signed-off-by: yzamir <kobi.zamir@gmail.com>
  - update makefile
  - update ci to use make file
  - remove travis config
  - update git ignore
  - update helper scripts

Signed-off-by: yzamir <kobi.zamir@gmail.com>
  - remove compiled examples that will only work on one version of python

Signed-off-by: yzamir <kobi.zamir@gmail.com>
  - add settings page
  - make the dashboard lighter

Signed-off-by: yzamir <kobi.zamir@gmail.com>
  - make a rose-common container based on ubi ptyhon
  - remove classes and methods that are not used (network, configuration)

Signed-off-by: yzamir <kobi.zamir@gmail.com>
  - move from twisted to asyncio
  - move from autobahn to websockets
  - add specific configuration files
  - add makefile
  - add dockerfile

Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
  - move from twisted to asyncio
  - move from autobahn to websockets
  - move files into a game directory

Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
@yaacov yaacov force-pushed the poc-separate-client-server branch 2 times, most recently from aba7194 to c98bcfb Compare September 12, 2023 12:44
Signed-off-by: yzamir <kobi.zamir@gmail.com>
@yaacov yaacov force-pushed the poc-separate-client-server branch 3 times, most recently from 53c301a to e8e4db7 Compare September 12, 2023 16:20
}

render() {
this.shadowRoot.innerHTML = `
Copy link
Member

@bennypowers bennypowers Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting take. I would have thought to not use shadow DOM in this case and just this.querySelector, but this is good too

You may choose to use a template element is a static init block, but this isn't reusable so the gains are few

If you'll stick with shadow DOM, use a constructible style sheet and adopted stylesheets instead of a link element

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennypowers that was a nice learning experience :-)
not using template, but separated the template and css from the main js component, WDYT ?

  - use module
  - use web component to render the form

Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
</head>
<body class="dark-theme">
<h2 class="heading">Set Game Drivers</h2>
<settings-form class="form" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom-elements can't be void

Suggested change
<settings-form class="form" />
<settings-form class="form"></settings-form>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, fixed, btw/ why did it work ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it was a hot topic 🔥
WICG/webcomponents#624

Signed-off-by: yzamir <kobi.zamir@gmail.com>
Signed-off-by: yzamir <kobi.zamir@gmail.com>
@yaacov
Copy link
Member Author

yaacov commented Jan 10, 2024

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:latest

and 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

@yaacov yaacov closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants