-
Notifications
You must be signed in to change notification settings - Fork 183
addition of Safearena Benchmark #345
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
998b506
5fec75d
5c5cc29
78f0e52
42c08b2
1e66af3
c288f29
e995149
fe707b5
965b74f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,7 +83,7 @@ | |
| strict=False, | ||
| retry_with_force=True, | ||
| demo_mode="off", | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| # all benchmarks are callables designed for lazy loading, i.e. `bench = DEFAULT_BENCHMARKS["miniwob_all"]()` | ||
|
|
@@ -258,4 +258,47 @@ | |
| ), | ||
| task_metadata=task_metadata("weblinx"), | ||
| ), | ||
| # from https://arxiv.org/abs/2503.04957 | ||
| "safearena_all": lambda: Benchmark( | ||
| name="safenarena_all", | ||
| high_level_action_set_args=DEFAULT_HIGHLEVEL_ACTION_SET_ARGS["webarena"], | ||
| is_multi_tab=True, | ||
| supports_parallel_seeds=False, | ||
| backends=["webarena"], | ||
| env_args_list=make_env_args_list_from_repeat_tasks( | ||
| task_list=task_list_from_metadata(metadata=task_metadata("safearena_all")), | ||
| max_steps=30, | ||
| n_repeats=1, | ||
|
Comment on lines
+268
to
+273
This comment was marked as resolved.
Sorry, something went wrong. |
||
| seeds_rng=np.random.RandomState(42), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Identical RNG Seeds Across Benchmarks
Tell me moreWhat is the issue?Multiple benchmarks are using the same fixed random seed (42) for generating environment arguments lists, which can lead to correlated test scenarios across different benchmarks. Why this mattersUsing the same seed across benchmarks reduces the diversity of test scenarios and may cause agents to overfit to specific patterns, failing to identify potential performance issues in edge cases. Suggested change ∙ Feature PreviewUse different seeds for each benchmark to ensure broader test coverage and prevent correlated scenarios. Consider using a seed generation function: def get_benchmark_seed(benchmark_name: str, base_seed: int = 42) -> int:
return hash(benchmark_name) + base_seedProvide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
| ), | ||
| task_metadata=task_metadata("safearena_all"), | ||
| ), | ||
| "safearena_harm": lambda: Benchmark( | ||
| name="safenarena_all", | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
| high_level_action_set_args=DEFAULT_HIGHLEVEL_ACTION_SET_ARGS["webarena"], | ||
| is_multi_tab=True, | ||
| supports_parallel_seeds=False, | ||
| backends=["webarena"], | ||
| env_args_list=make_env_args_list_from_repeat_tasks( | ||
| task_list=task_list_from_metadata(metadata=task_metadata("safearena_harm")), | ||
| max_steps=30, | ||
| n_repeats=1, | ||
| seeds_rng=np.random.RandomState(42), | ||
| ), | ||
| task_metadata=task_metadata("safearena_harm"), | ||
| ), | ||
| "safearena_safe": lambda: Benchmark( | ||
| name="safenarena_all", | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
| high_level_action_set_args=DEFAULT_HIGHLEVEL_ACTION_SET_ARGS["webarena"], | ||
| is_multi_tab=True, | ||
| supports_parallel_seeds=False, | ||
| backends=["webarena"], | ||
| env_args_list=make_env_args_list_from_repeat_tasks( | ||
| task_list=task_list_from_metadata(metadata=task_metadata("safearena_safe")), | ||
| max_steps=30, | ||
| n_repeats=1, | ||
| seeds_rng=np.random.RandomState(42), | ||
| ), | ||
| task_metadata=task_metadata("safearena_safe"), | ||
| ), | ||
| } | ||
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| [build-system] | ||
| requires = ["hatchling", "hatch-requirements-txt"] | ||
| build-backend = "hatchling.build" | ||
|
|
||
| [project] | ||
| name = "browsergym-safearena" | ||
| description = "Safearena: Web agent safety benchmark" | ||
| authors = [ | ||
| {name = "Ada Tur"}, | ||
| ] | ||
| readme = "README.md" | ||
| requires-python = ">3.10" | ||
| license = {text = "Apache-2.0"} | ||
| classifiers = [ | ||
| "Development Status :: 3 - Alpha", | ||
| "Programming Language :: Python :: 3", | ||
| "Operating System :: OS Independent", | ||
| "Intended Audience :: Science/Research", | ||
| "Topic :: Scientific/Engineering :: Artificial Intelligence", | ||
| "License :: OSI Approved :: Apache Software License", | ||
| ] | ||
| dynamic = ["dependencies", "version"] | ||
|
|
||
| [project.urls] | ||
| homepage = "https://github.com/ServiceNow/BrowserGym" | ||
|
|
||
| [tool.hatch.version] | ||
| path = "../core/src/browsergym/core/__init__.py" | ||
|
|
||
| [tool.hatch.metadata.hooks.requirements_txt] | ||
| files = ["requirements.txt"] | ||
|
|
||
| [tool.hatch.build.targets.wheel] | ||
| packages = ["src/browsergym"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| browsergym-core==0.13.3 | ||
| libwebarena==0.0.4 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import nltk | ||
|
|
||
| from browsergym.core.registration import register_task | ||
|
|
||
| from . import config, task | ||
|
|
||
| # download necessary tokenizer resources | ||
| # note: deprecated punkt -> punkt_tab https://github.com/nltk/nltk/issues/3293 | ||
| try: | ||
| nltk.data.find("tokenizers/punkt_tab") | ||
| except: | ||
| nltk.download("punkt_tab", quiet=True, raise_on_error=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhandled Download Failure
Tell me moreWhat is the issue?The code assumes the download will always succeed when raise_on_error=True, but network issues or permission problems could cause failures. Why this mattersIf the download fails, the program will continue without the required NLTK resources, potentially causing runtime errors in dependent code. Suggested change ∙ Feature PreviewAdd error handling for download failures: try:
nltk.data.find("tokenizers/punkt_tab")
except LookupError:
try:
nltk.download("punkt_tab", quiet=True, raise_on_error=True)
except Exception as e:
raise RuntimeError(f"Failed to download required NLTK resources: {e}")Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit.
Comment on lines
+9
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixed Resource Initialization with Task Registration
Tell me moreWhat is the issue?The initialization of NLTK resources is mixed with task registration logic, violating the Single Responsibility Principle. Why this mattersThis makes the code harder to maintain and test, as resource initialization is tightly coupled with task registration. It also makes it difficult to mock or bypass NLTK initialization during testing. Suggested change ∙ Feature PreviewExtract NLTK initialization into a separate function or module: def initialize_nltk_resources():
try:
nltk.data.find("tokenizers/punkt_tab")
except:
nltk.download("punkt_tab", quiet=True, raise_on_error=True)
# Call this function before task registration
initialize_nltk_resources()Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
|
||
| ALL_SAFEARENA_TASK_IDS = [] | ||
|
|
||
| # register all SafeArena benchmark | ||
| for task_id in config.TASK_IDS: | ||
| gym_id = f"safearena.{task_id}" | ||
| register_task( | ||
| gym_id, | ||
| task.GenericSafeArenaTask, | ||
| task_kwargs={"task_id": task_id}, | ||
| ) | ||
| ALL_SAFEARENA_TASK_IDS.append(gym_id) | ||
|
Comment on lines
+17
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lack of Task Registration Encapsulation
Tell me moreWhat is the issue?Task registration logic is not encapsulated in a dedicated function, making it harder to understand and maintain. Why this mattersThe current implementation makes it difficult to modify registration behavior or add pre/post registration hooks. It also makes the code less testable and harder to reuse. Suggested change ∙ Feature PreviewCreate a dedicated function for task registration: def register_safearena_tasks():
task_ids = []
for task_id in config.TASK_IDS:
gym_id = f"safearena.{task_id}"
register_task(
gym_id,
task.GenericSafeArenaTask,
task_kwargs={"task_id": task_id},
)
task_ids.append(gym_id)
return task_ids
ALL_SAFEARENA_TASK_IDS = register_safearena_tasks()Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| TASK_IDS = range(250) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lack of proper configuration management structure
Tell me moreWhat is the issue?Configuration data is directly hardcoded as a global variable without proper encapsulation or configuration management structure. Why this mattersHardcoding configuration values makes the code rigid and difficult to maintain. Changes require code modifications rather than configuration updates, and there's no clear separation between configuration and code logic. Suggested change ∙ Feature PreviewImplement a proper configuration management system, for example: from dataclasses import dataclass
from typing import Sequence
@dataclass
class SafeArenaConfig:
task_ids: Sequence[int] = tuple(range(250))
# Usage:
config = SafeArenaConfig()Or use a configuration file (yaml/json) with a parser class. Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| import logging | ||
| import os | ||
| import time | ||
|
|
||
| import playwright.sync_api | ||
| import requests | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| ENV_VARS = ("SHOPPING", "SHOPPING_ADMIN", "REDDIT", "GITLAB", "HOMEPAGE") | ||
|
|
||
|
|
||
| class SafeArenaInstance: | ||
| """ | ||
| Utility class to access a SafeArena instance. | ||
|
|
||
| """ | ||
|
|
||
| RESET_URL_VAR = "SA_FULL_RESET" # used by full_reset() | ||
|
|
||
| def __init__( | ||
| self, | ||
| ) -> None: | ||
|
|
||
| # setup safearena environment variables (safearena will read those on import) | ||
| append_wa = lambda x: f"SA_{x}" | ||
| for key in ENV_VARS: | ||
| assert append_wa(key) in os.environ, ( | ||
| f"Environment variable {append_wa(key)} missing.\n" | ||
| + "Please set the following environment variables to use SafeArena through BrowserGym:\n" | ||
| + "\n".join([append_wa(x) for x in ENV_VARS]) | ||
| ) | ||
| os.environ[key] = os.environ[append_wa(key)] | ||
|
|
||
| # import safearena on instanciation | ||
| from webarena.browser_env.env_config import ( | ||
| ACCOUNTS, | ||
| GITLAB, | ||
| HOMEPAGE, | ||
| REDDIT, | ||
| SHOPPING, | ||
| SHOPPING_ADMIN, | ||
| ) | ||
|
|
||
| self.urls = { | ||
| "reddit": REDDIT, | ||
| "gitlab": GITLAB, | ||
| "shopping": SHOPPING, | ||
| "shopping_admin": SHOPPING_ADMIN | ||
| } | ||
| self.home_url = HOMEPAGE | ||
|
|
||
| self.credentials = ACCOUNTS | ||
|
|
||
| def full_reset(self, skip_if_not_set: bool = True): | ||
| base_url = os.environ.get(self.RESET_URL_VAR, None) | ||
|
|
||
| if not base_url: | ||
| # check for reset URL | ||
| logger.error( | ||
| f"Environment variable {self.RESET_URL_VAR} is missing or empty, required for a full instance reset." | ||
| ) | ||
| if skip_if_not_set: | ||
| logger.warning( | ||
| f"Skipping automated reset. Make sure the instance has been manually reset." | ||
| ) | ||
| else: | ||
| raise RuntimeError(f"Could not reset instance, aborting.") | ||
|
|
||
| else: | ||
| # reset the instance | ||
| reset_url = f"{base_url}/reset" | ||
| status_url = f"{base_url}/status" | ||
|
|
||
| logger.info( | ||
| f"Initiating {self.__class__.__name__} instance reset on URL {reset_url}. Should take between 200 - 500 seconds to restart." | ||
| ) | ||
|
|
||
| # trigger instance reset | ||
| response = requests.get(reset_url) | ||
| match response.status_code: | ||
| case 200: | ||
| logger.info(f"Reset started.") | ||
| case 418: | ||
| logger.warning("Reset was already running.") | ||
| case _: | ||
| raise Exception( | ||
| f"{self.__class__.__name__} reset request {reset_url} failed ({response.status_code}): {response.text}" | ||
| ) | ||
|
|
||
| # wait until reset complete | ||
| retry_after = 20 # 20 seconds wait between status checks | ||
| timeout = 10 * 60 # 10 minutes timeout | ||
| start_time = time.time() | ||
| while True: | ||
| # request instance status | ||
| response = requests.get(status_url) | ||
| # check for server error | ||
| if response.status_code != 200: | ||
| raise Exception( | ||
| f"{self.__class__.__name__} status request {status_url} failed ({response.status_code}): {response.text}" | ||
| ) | ||
| # check for readiness | ||
| if response.text == "Ready for duty!": | ||
| break | ||
| # check for timeout | ||
| time_elapsed = time.time() - start_time | ||
| logger.info(f"Reset still running after {time_elapsed:.0f} seconds...") | ||
| if time_elapsed > timeout: | ||
| raise Exception( | ||
| f"Reset still running after {time_elapsed} seconds (> {timeout}), aborting." | ||
| ) | ||
| # wait a bit before next retry | ||
| time.sleep(retry_after) | ||
|
|
||
| # warm-start the instance (navigate to every domain) | ||
| retries_left = 3 | ||
| while retries_left: | ||
| retries_left -= 1 | ||
| try: | ||
| self._check_is_reachable( | ||
| timeout=60 | ||
| ) # 60 seconds, warming up after reset might be slow | ||
| break | ||
| except Exception as e: | ||
| if not retries_left: | ||
| raise | ||
| logger.info( | ||
| f"Instance unresponsive after reset, retrying ({retries_left} retries left)\n{e}" | ||
| ) | ||
|
|
||
| def check_status(self): | ||
| """ | ||
| Check the status of the instance. Raises an error if the instance is not ready to be used. | ||
|
|
||
| """ | ||
| self._check_is_reachable(timeout=10) # 10 seconds | ||
|
|
||
| def _check_is_reachable(self, timeout: int): | ||
| """ | ||
| Test that every website is reachable. | ||
|
|
||
| """ | ||
| for site, url in self.urls.items(): | ||
| try: | ||
| requests.get(url, timeout=timeout) | ||
| except (requests.exceptions.ConnectionError, requests.exceptions.Timeout): | ||
| raise RuntimeError( | ||
| f'SafeArena site "{site}" ({url}) is not reacheable. Please check the URL.' | ||
| ) | ||
|
Comment on lines
+151
to
+156
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incomplete request exception handling
Tell me moreWhat is the issue?The error handling for the request check is too specific, potentially missing other request exceptions like InvalidURL, InvalidSchema, or TooManyRedirects. Why this mattersOther request exceptions could occur and would propagate uncaught, making debugging harder since the error message wouldn't indicate which site failed. Suggested change ∙ Feature PreviewUse requests.exceptions.RequestException to catch all request-related errors: try:
requests.get(url, timeout=timeout)
except requests.exceptions.RequestException as e:
raise RuntimeError(
f'SafeArena site "{site}" ({url}) is not reacheable: {str(e)}'
)Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
|
||
| def ui_login(self, site: str, page: playwright.sync_api.Page): | ||
| """ | ||
| Should only be called once per site (expects user to be logged out). | ||
| """ | ||
|
|
||
| url = self.urls[site] | ||
|
|
||
| # open a new page (tab) to perform the login | ||
| page = page.context.new_page() | ||
|
|
||
| match site: | ||
| case "reddit": | ||
| username = self.credentials[site]["username"] | ||
| password = self.credentials[site]["password"] | ||
|
|
||
| page.goto(f"{url}") | ||
| page.get_by_role("link", name="Log in").click() | ||
| page.get_by_label("Username").fill(username) | ||
| page.get_by_label("Password").fill(password) | ||
| page.get_by_role("button", name="Log in").click() | ||
|
|
||
| case "gitlab": | ||
| username = self.credentials[site]["username"] | ||
| password = self.credentials[site]["password"] | ||
|
|
||
| page.goto(f"{url}/users/sign_in") | ||
| page.get_by_label("Username or email").fill(username) | ||
| page.get_by_label("Password").fill(password) | ||
| page.get_by_role("button", name="Sign in").click() | ||
|
|
||
| case "shopping": | ||
| username = self.credentials[site]["username"] | ||
| password = self.credentials[site]["password"] | ||
|
|
||
| page.goto(f"{url}/customer/account/login/") | ||
| page.get_by_label("Email", exact=True).fill(username) | ||
| page.get_by_label("Password", exact=True).fill(password) | ||
| page.get_by_role("button", name="Sign In").click() | ||
|
|
||
| case "shopping_admin": | ||
| username = self.credentials[site]["username"] | ||
| password = self.credentials[site]["password"] | ||
|
|
||
| page.goto(url) | ||
| page.get_by_label("Username").fill(username) | ||
| page.get_by_label("Password").fill(password) | ||
| page.get_by_role("button", name="Sign in").click() | ||
|
|
||
|
|
||
| case _: | ||
| raise ValueError | ||
|
|
||
| # release login page | ||
| page.close() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in SafeArena All Benchmark Name
Tell me more
What is the issue?
The benchmark name contains a typo: 'safenarena_all' instead of 'safearena_all'.
Why this matters
This inconsistency in naming could lead to problems with benchmark identification and could break functionality that relies on exact name matching.
Suggested change ∙ Feature Preview
Correct the spelling in the name parameter:
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.