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

Carcass for browser tool: client, server, servicer and empty web env #466

Merged

Conversation

MariaIzobava
Copy link
Collaborator

@MariaIzobava MariaIzobava commented Sep 20, 2024

This PR contains:

  • New features
  • Changes to dev-tools e.g. CI config / github tooling
  • Docs
  • Bug fixes
  • Code refactor

What is the current behavior? (You can also link to an open issue here)

Open issue: #391

What is the new behavior?

New files

  1. web_server.py - spins a new gRPC server
  2. dm_env_servicer.py - an implementation for EnvironmentService based on dm_env_rpc protocol
    • The servicer can create a single world named "WebBrowser" and will allow a single client joining it at a time.
  3. web_environment.py - web environment which should instantiate playwright library (will be completed in the next commit)
  4. web_client.py - a simple client to interract with the the server. It does the following:
    1. Instantiates the connection with the server
    2. Joins the "WebBrowser" world
    3. Makes a single "step" (i.e. sends a single command to the browser)
    4. Receives the result of the command in form of observations, prints them to stdout and closes the connection
  5. mock_environment.py - a Mock class for WebEnvironment to be used in tests
  6. test_dm_env_servicer.py - test for dm_env_servicer.py which uses the MockEnvironment
    • The test makes sure the servicer is implemented and acts according to the [dm_env_rpc protocol] requirements
  7. Dockerfile - instantiates all required deps, copies the files and spins the server

Testing

The test file won't be picked up by make test since it's not under tests/ dir (let me know if that should be changed).
Manual run with pytest succeeds:
Screenshot 2024-09-20 at 12 03 13

Usage

For now, since WebEnvironment class is not fully implemented, each command will be treated as "invalid command":
Screenshot 2024-09-20 at 12 02 37

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

Other information:

@@ -0,0 +1,21 @@
# Base docker build file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we thinking that the container that hosts the web browser will be dedicated to just the web browser? If so, that would result in an extra container per-sample compared to integrating it. That's is fine by me if you think that's the best approach, just wanted to clarify my understanding and flag the additional resource usage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I've thought about this further I very much like the idea of the web_browser having its own container. This will make "importing" it into challenges very easy (just add a service to compose.yaml). I also think that compared to the overhead of Chromium the overhead of a container will be pretty low. So no reservations at all about this approach!

@MariaIzobava
Copy link
Collaborator Author

@jjallaire-aisi, please let me know if I should create a follow up PR with the rest of the code already. I'm not sure what is the general strategy with sending lots of code for review: is it better to do at once or in chunks? I have the rest of the implementation ready-to-go.

@jjallaire-aisi
Copy link
Collaborator

Hi @MariaIzobava , yes, let's merge this PR first then do another one for the rest of the feature. Will do that momentarily....

@jjallaire jjallaire merged commit 0df08df into UKGovernmentBEIS:main Sep 26, 2024
7 of 9 checks passed
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