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

Replace browsergym with browsergym-core to reduce dependencies #5340

Closed
wants to merge 2 commits into from

Conversation

neubig
Copy link
Contributor

@neubig neubig commented Dec 1, 2024

This PR replaces the dependency on browsergym with browsergym-core to reduce unneeded dependencies. The full browsergym package includes many additional dependencies that are not needed for our use case.

Changes:

  • Changed dependency from browsergym to browsergym-core in pyproject.toml
  • Updated poetry.lock to remove unneeded dependencies

All tests pass successfully, confirming that the core browsergym functionality works as expected.


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:355c208-nikolaik   --name openhands-app-355c208   docker.all-hands.dev/all-hands-ai/openhands:355c208

Copy link
Contributor Author

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Tests are now failing due to a missing dependency on "joblib". I expect that this was because joblib was included by browsergym, but it not explicitly included in our pyproject.toml file. We should also include a relevant version of joblib in pyproject.toml to adjust for this.

@neubig neubig added the fix-me Attempt to fix this issue with OpenHands label Dec 1, 2024
Copy link
Contributor

github-actions bot commented Dec 1, 2024

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor

New OpenHands update

Copy link
Contributor

github-actions bot commented Dec 1, 2024

The workflow to fix this issue encountered an error. Please check the workflow logs for more information.

@enyst
Copy link
Collaborator

enyst commented Dec 1, 2024

@neubig Sorry, we can continue to fix it here, but just for discussion, have you seen this?


That PR is in the stage "thinking about" 😅 because:

  • the envs can be part of the 'evaluation' poetry group, which is not normally run in CI
      • I moved miniwob etc to 'evaluation' group on that branch
    • CI is or should be run with poetry install --without evaluation,llama-index
  • but there's a browsing test with miniwob that will fail as long as they're not installed
  • which I had some trouble disabling in a way that remains very visible (= not prone to be forgotten)
  • this isn't a new problem, we have it already with llama-index, same thing, disabled in normal CI, should not be forgotten, needs a place to run and us keeping an eye on

So my current theory about how to fix this, is to move both the llama-index and evaluation groups tests to some other pipeline like we now have the integration tests one:

@neubig
Copy link
Contributor Author

neubig commented Dec 1, 2024

Hey @enyst , I indeed had not seen that. Apologies! I'll look through carefully when I have a moment.

@enyst enyst mentioned this pull request Dec 2, 2024
1 task
@neubig
Copy link
Contributor Author

neubig commented Dec 2, 2024

Closing in favor of #5242

@neubig neubig closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix-me Attempt to fix this issue with OpenHands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants