From 310fb858e522c4d8ed73c1483c1b85795cbf6c05 Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Tue, 16 Dec 2025 09:54:20 -0800 Subject: [PATCH 1/2] remove auth ini logic --- development/CONTRIBUTING.md | 15 +- docs/cli_reference/cli_overview.mdx | 2 +- eval_protocol/auth.py | 242 +---------------- eval_protocol/cli.py | 34 --- eval_protocol/cli_commands/upload.py | 4 +- eval_protocol/cli_commands/utils.py | 19 +- tests/test_auth.py | 383 +++------------------------ 7 files changed, 65 insertions(+), 634 deletions(-) diff --git a/development/CONTRIBUTING.md b/development/CONTRIBUTING.md index 3d421cc8..d715fb69 100644 --- a/development/CONTRIBUTING.md +++ b/development/CONTRIBUTING.md @@ -106,24 +106,15 @@ export FIREWORKS_API_BASE="https://dev.api.fireworks.ai" # If targeting dev API **B. Configuration File (Lower Priority)** -If environment variables are not set, Eval Protocol will attempt to read credentials from `~/.fireworks/auth.ini`. - -Create or ensure the file `~/.fireworks/auth.ini` exists with the following format: -```ini -[fireworks] -api_key = YOUR_FIREWORKS_API_KEY -account_id = YOUR_FIREWORKS_ACCOUNT_ID -``` -Replace with your actual development credentials if using this method. +Eval Protocol does not read `~/.fireworks/auth.ini` (or any firectl profiles). Use environment variables instead. **Credential Sourcing Order:** Eval Protocol prioritizes credentials as follows: -1. Environment Variables (`FIREWORKS_API_KEY`, `FIREWORKS_ACCOUNT_ID`) -2. `~/.fireworks/auth.ini` configuration file +1. Environment Variables (`FIREWORKS_API_KEY`, optional `FIREWORKS_ACCOUNT_ID`) **Purpose of Credentials:** * `FIREWORKS_API_KEY`: Authenticates your requests to the Fireworks AI service. -* `FIREWORKS_ACCOUNT_ID`: Identifies your account for operations like managing evaluators. It specifies *where* (under which account) an operation should occur. +* `FIREWORKS_ACCOUNT_ID`: Identifies your account for operations like managing evaluators. Typically this is derived automatically from `FIREWORKS_API_KEY` via the `verifyApiKey` endpoint. * `FIREWORKS_API_BASE`: Allows targeting different API environments (e.g., development, staging). **Other Environment Variables:** diff --git a/docs/cli_reference/cli_overview.mdx b/docs/cli_reference/cli_overview.mdx index 05f63717..e9303d2f 100644 --- a/docs/cli_reference/cli_overview.mdx +++ b/docs/cli_reference/cli_overview.mdx @@ -329,7 +329,7 @@ The CLI recognizes the following environment variables: - `FIREWORKS_API_KEY`: Your Fireworks API key (required for deployment operations) - `FIREWORKS_API_BASE`: Base URL for the Fireworks API (defaults to `https://api.fireworks.ai`) -- `FIREWORKS_ACCOUNT_ID`: Your Fireworks account ID (optional, can be configured in auth.ini) +- `FIREWORKS_ACCOUNT_ID`: Your Fireworks account ID (optional; typically derived automatically from `FIREWORKS_API_KEY`) - `MODEL_AGENT`: Default agent model to use (e.g., "openai/gpt-4o-mini") - `MODEL_SIM`: Default simulation model to use (e.g., "openai/gpt-3.5-turbo") diff --git a/eval_protocol/auth.py b/eval_protocol/auth.py index f1d6c922..27781468 100644 --- a/eval_protocol/auth.py +++ b/eval_protocol/auth.py @@ -1,222 +1,24 @@ -import configparser import logging import os -from pathlib import Path -from typing import Dict, Optional # Added Dict +from typing import Optional import requests logger = logging.getLogger(__name__) -# Default locations (used for tests and as fallback). Actual resolution is dynamic via _get_auth_ini_file(). -FIREWORKS_CONFIG_DIR = Path.home() / ".fireworks" -AUTH_INI_FILE = FIREWORKS_CONFIG_DIR / "auth.ini" - - -def _get_profile_base_dir() -> Path: - """ - Resolve the Fireworks configuration base directory following firectl behavior: - - Default: ~/.fireworks - - If FIREWORKS_PROFILE is set and non-empty: ~/.fireworks/profiles/ - """ - profile_name = os.environ.get("FIREWORKS_PROFILE", "").strip() - base_dir = Path.home() / ".fireworks" - if profile_name: - base_dir = base_dir / "profiles" / profile_name - return base_dir - - -def _get_auth_ini_file() -> Path: - """ - Determine the auth.ini file path. - Priority: - 1) FIREWORKS_AUTH_FILE env var when set - 2) ~/.fireworks[/profiles/]/auth.ini (profile driven) - """ - auth_file_env = os.environ.get("FIREWORKS_AUTH_FILE") - if auth_file_env: - return Path(auth_file_env) - return _get_profile_base_dir() / "auth.ini" - - -def _is_profile_active() -> bool: - """ - Returns True if a specific profile or explicit auth file is active. - In this case, profile-based credentials should take precedence over env vars. - """ - if os.environ.get("FIREWORKS_AUTH_FILE"): - return True - prof = os.environ.get("FIREWORKS_PROFILE", "").strip() - return bool(prof) - - -def _parse_simple_auth_file(file_path: Path) -> Dict[str, str]: - """ - Parses an auth file with simple key=value lines. - Handles comments starting with # or ;. - Strips whitespace and basic quotes from values. - """ - creds = {} - if not file_path.exists(): - return creds - try: - with open(file_path, "r", encoding="utf-8") as f: - for line in f: - line = line.strip() - if not line or line.startswith("#") or line.startswith(";"): - continue - if "=" in line: - key, value = line.split("=", 1) - key = key.strip() - value = value.strip() - # Remove surrounding quotes if present - if value and ( - (value.startswith('"') and value.endswith('"')) - or (value.startswith("'") and value.endswith("'")) - ): - value = value[1:-1] - - if key in ["api_key", "account_id"] and value: - creds[key] = value - except Exception as e: - logger.warning("Error during simple parsing of %s: %s", str(file_path), e) - return creds - - -def _get_credential_from_config_file(key_name: str) -> Optional[str]: - """ - Helper to get a specific credential (api_key or account_id) from auth.ini. - Tries simple parsing first, then configparser. - """ - auth_ini_path = _get_auth_ini_file() - if not auth_ini_path.exists(): - return None - - # 1. Try simple key-value parsing first - simple_creds = _parse_simple_auth_file(auth_ini_path) - if key_name in simple_creds: - logger.debug("Using %s from simple key-value parsing of %s.", key_name, str(auth_ini_path)) - return simple_creds[key_name] - - # 2. Fallback to configparser if not found via simple parsing or if simple parsing failed - # This path will also generate the "no section headers" warning if applicable, - # but only if simple parsing didn't yield the key. - try: - config = configparser.ConfigParser() - config.read(auth_ini_path) - - # Try [fireworks] section - if "fireworks" in config and config.has_option("fireworks", key_name): - value_from_file = config.get("fireworks", key_name) - if value_from_file: - logger.debug("Using %s from [fireworks] section in %s.", key_name, str(auth_ini_path)) - return value_from_file - - # Try default section (configparser might place items without section header here) - if config.has_option(config.default_section, key_name): - value_from_default = config.get(config.default_section, key_name) - if value_from_default: - logger.debug( - "Using %s from default section [%s] in %s.", - key_name, - config.default_section, - str(auth_ini_path), - ) - return value_from_default - - except configparser.MissingSectionHeaderError: - # This error implies the file is purely key-value, which simple parsing should have handled. - # If simple parsing failed to get the key, then it's likely not there or malformed. - logger.debug("%s has no section headers, and simple parsing did not find %s.", str(auth_ini_path), key_name) - except configparser.Error as e_config: - logger.warning("Configparser error reading %s for %s: %s", str(auth_ini_path), key_name, e_config) - except Exception as e_general: - logger.warning("Unexpected error reading %s for %s: %s", str(auth_ini_path), key_name, e_general) - - return None - - -def _get_credentials_from_config_file() -> Dict[str, Optional[str]]: - """ - Retrieve both api_key and account_id from auth.ini with a single read/parse. - Tries simple parsing first for both keys, then falls back to configparser for any missing ones. - Returns a dict with up to two keys: 'api_key' and 'account_id'. - """ - results: Dict[str, Optional[str]] = {} - auth_ini_path = _get_auth_ini_file() - if not auth_ini_path.exists(): - return results - - # 1) Simple key=value parsing - try: - simple_creds = _parse_simple_auth_file(auth_ini_path) - if "api_key" in simple_creds and simple_creds["api_key"]: - results["api_key"] = simple_creds["api_key"] - if "account_id" in simple_creds and simple_creds["account_id"]: - results["account_id"] = simple_creds["account_id"] - if "api_key" in results and "account_id" in results: - return results - except Exception as e: - logger.warning("Error during simple parsing of %s: %s", str(auth_ini_path), e) - - # 2) ConfigParser for any missing keys - try: - config = configparser.ConfigParser() - config.read(auth_ini_path) - for key_name in ("api_key", "account_id"): - if key_name in results and results[key_name]: - continue - if "fireworks" in config and config.has_option("fireworks", key_name): - value_from_file = config.get("fireworks", key_name) - if value_from_file: - results[key_name] = value_from_file - continue - if config.has_option(config.default_section, key_name): - value_from_default = config.get(config.default_section, key_name) - if value_from_default: - results[key_name] = value_from_default - except configparser.MissingSectionHeaderError: - # Purely key=value file without section headers; simple parsing should have handled it already. - logger.debug("%s has no section headers; falling back to simple parsing results.", str(auth_ini_path)) - except configparser.Error as e_config: - logger.warning("Configparser error reading %s: %s", str(auth_ini_path), e_config) - except Exception as e_general: - logger.warning("Unexpected error reading %s: %s", str(auth_ini_path), e_general) - - return results - def get_fireworks_api_key() -> Optional[str]: """ Retrieves the Fireworks API key. - The key is sourced in the following order: - 1. FIREWORKS_API_KEY environment variable. - 2. 'api_key' from the [fireworks] section of ~/.fireworks/auth.ini. - Returns: The API key if found, otherwise None. """ - # If a profile is active, prefer profile file first, then env - if _is_profile_active(): - api_key_from_file = _get_credential_from_config_file("api_key") - if api_key_from_file: - return api_key_from_file - api_key = os.environ.get("FIREWORKS_API_KEY") - if api_key: - logger.debug("Using FIREWORKS_API_KEY from environment variable (profile active but file missing).") - return api_key - else: - # Default behavior: env overrides file - api_key = os.environ.get("FIREWORKS_API_KEY") - if api_key: - logger.debug("Using FIREWORKS_API_KEY from environment variable.") - return api_key - api_key_from_file = _get_credential_from_config_file("api_key") - if api_key_from_file: - return api_key_from_file - - logger.debug("Fireworks API key not found in environment variables or auth.ini.") + api_key = os.environ.get("FIREWORKS_API_KEY") + if api_key and api_key.strip(): + logger.debug("Using FIREWORKS_API_KEY from environment variable.") + return api_key.strip() + logger.debug("Fireworks API key not found in environment variables.") return None @@ -226,36 +28,18 @@ def get_fireworks_account_id() -> Optional[str]: The Account ID is sourced in the following order: 1. FIREWORKS_ACCOUNT_ID environment variable. - 2. 'account_id' from the [fireworks] section of ~/.fireworks/auth.ini. - 3. If an API key is available (env or auth.ini), resolve via verifyApiKey. + 2. If an API key is available (env), resolve via verifyApiKey. Returns: The Account ID if found, otherwise None. """ - # If a profile is active, prefer profile file first, then env - if _is_profile_active(): - creds = _get_credentials_from_config_file() - account_id_from_file = creds.get("account_id") - if account_id_from_file: - return account_id_from_file - account_id = os.environ.get("FIREWORKS_ACCOUNT_ID") - if account_id: - logger.debug("Using FIREWORKS_ACCOUNT_ID from environment variable (profile active but file missing).") - return account_id - else: - # Default behavior: env overrides file - account_id = os.environ.get("FIREWORKS_ACCOUNT_ID") - if account_id: - logger.debug("Using FIREWORKS_ACCOUNT_ID from environment variable.") - return account_id - creds = _get_credentials_from_config_file() - account_id_from_file = creds.get("account_id") - if account_id_from_file: - return account_id_from_file + account_id = os.environ.get("FIREWORKS_ACCOUNT_ID") + if account_id and account_id.strip(): + logger.debug("Using FIREWORKS_ACCOUNT_ID from environment variable.") + return account_id.strip() - # 3) Fallback: if API key is present, attempt to resolve via verifyApiKey (env or auth.ini) + # Fallback: if API key is present, attempt to resolve via verifyApiKey (env) try: - # Intentionally use get_fireworks_api_key to centralize precedence (env vs file) api_key_for_verify = get_fireworks_api_key() if api_key_for_verify: resolved = verify_api_key_and_get_account_id(api_key=api_key_for_verify, api_base=get_fireworks_api_base()) @@ -265,7 +49,7 @@ def get_fireworks_account_id() -> Optional[str]: except Exception as e: logger.debug("Failed to resolve FIREWORKS_ACCOUNT_ID via verifyApiKey: %s", e) - logger.debug("Fireworks Account ID not found in environment variables, auth.ini, or via verifyApiKey.") + logger.debug("Fireworks Account ID not found in environment variables or via verifyApiKey.") return None diff --git a/eval_protocol/cli.py b/eval_protocol/cli.py index e8125390..8aa7fdb0 100644 --- a/eval_protocol/cli.py +++ b/eval_protocol/cli.py @@ -36,10 +36,6 @@ def parse_args(args=None): """Parse command line arguments""" parser = argparse.ArgumentParser(description="eval-protocol: Tools for evaluation and reward modeling") parser.add_argument("--verbose", "-v", action="store_true", help="Enable verbose logging") - parser.add_argument( - "--profile", - help="Fireworks profile to use (reads ~/.fireworks/profiles//auth.ini and settings.ini)", - ) parser.add_argument( "--server", help="Fireworks API server hostname or URL (e.g., dev.api.fireworks.ai or https://dev.api.fireworks.ai)", @@ -537,38 +533,8 @@ def _extract_flag_value(argv_list, flag_name): return tok.split("=", 1)[1] return None - pre_profile = _extract_flag_value(raw_argv, "--profile") pre_server = _extract_flag_value(raw_argv, "--server") - # Handle Fireworks profile selection early so downstream modules see the env - profile = pre_profile - if profile: - try: - os.environ["FIREWORKS_PROFILE"] = profile - # Mirror firectl behavior: ~/.fireworks[/profiles/] - base_dir = Path.home() / ".fireworks" - if profile: - base_dir = base_dir / "profiles" / profile - os.makedirs(str(base_dir), mode=0o700, exist_ok=True) - - # Provide helpful env hints for consumers (optional) - os.environ["FIREWORKS_AUTH_FILE"] = str(base_dir / "auth.ini") - os.environ["FIREWORKS_SETTINGS_FILE"] = str(base_dir / "settings.ini") - logger.debug("Using Fireworks profile '%s' at %s", profile, base_dir) - except OSError as e: - logger.warning("Failed to initialize Fireworks profile '%s': %s", profile, e) - - # Proactively resolve and export account_id from the active profile to avoid stale .env overrides - try: - from eval_protocol.auth import get_fireworks_account_id as _resolve_account_id - - resolved_account = _resolve_account_id() - if resolved_account: - os.environ["FIREWORKS_ACCOUNT_ID"] = resolved_account - logger.debug("Resolved account_id from profile '%s': %s", profile, resolved_account) - except Exception as e: # noqa: B902 - logger.debug("Unable to resolve account_id from profile '%s': %s", profile, e) - # Handle Fireworks server selection early server = pre_server if server: diff --git a/eval_protocol/cli_commands/upload.py b/eval_protocol/cli_commands/upload.py index 33e0ed2f..c978d48c 100644 --- a/eval_protocol/cli_commands/upload.py +++ b/eval_protocol/cli_commands/upload.py @@ -248,7 +248,9 @@ def upload_command(args: argparse.Namespace) -> int: print(f"Warning: Failed to create/update {secret_name} secret on Fireworks.") else: if not fw_account_id: - print("Warning: FIREWORKS_ACCOUNT_ID not found; cannot register secrets.") + print( + "Warning: Could not resolve Fireworks account id from FIREWORKS_API_KEY; cannot register secrets." + ) if not secrets_from_file: print("Warning: No API keys found in environment or .env file; no secrets to register.") except Exception as e: diff --git a/eval_protocol/cli_commands/utils.py b/eval_protocol/cli_commands/utils.py index f6e2525f..d49b97e9 100644 --- a/eval_protocol/cli_commands/utils.py +++ b/eval_protocol/cli_commands/utils.py @@ -397,14 +397,19 @@ def _normalize_evaluator_id(evaluator_id: str) -> str: def _ensure_account_id() -> Optional[str]: """Resolve and cache FIREWORKS_ACCOUNT_ID if possible.""" - account_id = get_fireworks_account_id() + account_id = os.environ.get("FIREWORKS_ACCOUNT_ID") + if account_id and account_id.strip(): + return account_id.strip() + api_key = get_fireworks_api_key() - if not account_id and api_key: - resolved = verify_api_key_and_get_account_id(api_key=api_key, api_base=get_fireworks_api_base()) - if resolved: - os.environ["FIREWORKS_ACCOUNT_ID"] = resolved - return resolved - return account_id + if not api_key: + return None + + resolved = verify_api_key_and_get_account_id(api_key=api_key, api_base=get_fireworks_api_base()) + if resolved: + os.environ["FIREWORKS_ACCOUNT_ID"] = resolved + return resolved + return None def _extract_terminal_segment(resource_name: str) -> str: diff --git a/tests/test_auth.py b/tests/test_auth.py index 72dd54ca..ef280557 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -1,44 +1,31 @@ -import configparser # Import the original for type hinting if needed, but not for spec. import os - -# Import the original ConfigParser for use in spec if absolutely necessary, -# though direct configuration of the mock instance is preferred. -from configparser import ConfigParser as OriginalConfigParser -from pathlib import Path -from unittest.mock import MagicMock, mock_open, patch +from unittest.mock import patch import pytest -# Import the SUT from eval_protocol.auth import ( - AUTH_INI_FILE, get_fireworks_account_id, get_fireworks_api_key, + verify_api_key_and_get_account_id, ) -# Test data + TEST_ENV_API_KEY = "test_env_api_key_123" TEST_ENV_ACCOUNT_ID = "test_env_account_id_456" -INI_API_KEY = "ini_api_key_abc" -INI_ACCOUNT_ID = "ini_account_id_def" @pytest.fixture(autouse=True) def clear_env_vars_fixture(): - env_vars_to_clear = ["FIREWORKS_API_KEY", "FIREWORKS_ACCOUNT_ID"] + env_vars_to_clear = ["FIREWORKS_API_KEY", "FIREWORKS_ACCOUNT_ID", "FIREWORKS_API_BASE"] original_values = {var: os.environ.get(var) for var in env_vars_to_clear} for var in env_vars_to_clear: - if var in os.environ: - del os.environ[var] + os.environ.pop(var, None) yield for var, value in original_values.items(): - if value is not None: + if value is None: + os.environ.pop(var, None) + else: os.environ[var] = value - elif var in os.environ: - del os.environ[var] - - -# --- Tests for get_fireworks_api_key --- def test_get_api_key_from_env(): @@ -46,160 +33,8 @@ def test_get_api_key_from_env(): assert get_fireworks_api_key() == TEST_ENV_API_KEY -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") # Mocks the ConfigParser class -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) # Ensure simple parse finds nothing -def test_get_api_key_from_ini(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - # Configure the instance that configparser.ConfigParser() will return - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - - # Simulate key found in [fireworks] section - def has_option_fireworks_true(section, option): - if section == "fireworks": - return option == "api_key" - return False # Not in default or other sections for this test - - def get_fireworks_value(section, option): - if section == "fireworks" and option == "api_key": - return INI_API_KEY - raise configparser.NoOptionError(option, section) - - mock_parser_instance.has_option.side_effect = has_option_fireworks_true - mock_parser_instance.get.side_effect = get_fireworks_value - # Ensure 'fireworks' section itself exists - mock_parser_instance.__contains__.side_effect = lambda item: item == "fireworks" # For "fireworks" in config check - - with patch( - "builtins.open", mock_open(read_data="[fireworks]\napi_key = foo") - ): # Actual read_data not used by mock parser - assert get_fireworks_api_key() == INI_API_KEY - - mock_path_exists.assert_called_once_with() - mock_ConfigParser_class.assert_called_once_with() # Class was instantiated - mock_parser_instance.read.assert_called_once_with(AUTH_INI_FILE) - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -def test_get_api_key_env_overrides_ini(): - os.environ["FIREWORKS_API_KEY"] = TEST_ENV_API_KEY - with ( - patch("pathlib.Path.exists") as mock_path_exists, - patch("configparser.ConfigParser") as mock_ConfigParser_class, - patch("eval_protocol.auth._parse_simple_auth_file") as mock_parse_simple, - ): - assert get_fireworks_api_key() == TEST_ENV_API_KEY - mock_parse_simple.assert_not_called() # Env var should be checked first - mock_path_exists.assert_not_called() - mock_ConfigParser_class.assert_not_called() - - -@patch("pathlib.Path.exists", return_value=False) -def test_get_api_key_not_found(mock_path_exists): - # Ensure _parse_simple_auth_file is also considered if Path.exists is True but file is empty/no key - with patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) as mock_parse_simple: - assert get_fireworks_api_key() is None - # _get_credential_from_config_file checks AUTH_INI_FILE.exists() first - # if it's false, _parse_simple_auth_file won't be called by it. - # However, get_fireworks_api_key calls _get_credential_from_config_file, - # which itself calls _parse_simple_auth_file if AUTH_INI_FILE.exists(). - # This test specifically tests when AUTH_INI_FILE does *not* exist. - mock_parse_simple.assert_not_called() # Because AUTH_INI_FILE.exists() is False - mock_path_exists.assert_called_once_with() - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) # Simple parse finds nothing -def test_get_api_key_ini_exists_no_section(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - # Simulate MissingSectionHeaderError to trigger fallback within configparser logic (though simple parse is first) - mock_parser_instance.read.side_effect = configparser.MissingSectionHeaderError("file", 1, "line") - - with patch( - "builtins.open", # This mock_open is for the configparser's attempt if it were reached - mock_open(read_data="other_key = some_val_but_no_section_header\nanother=val"), - ): - assert get_fireworks_api_key() is None - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_api_key_ini_exists_no_key_option(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - - mock_parser_instance.__contains__.side_effect = lambda item: item == "fireworks" - mock_parser_instance.has_option.side_effect = lambda section, option: False - - with patch("builtins.open", mock_open(read_data="[fireworks]\nsome_other_key=foo")): - assert get_fireworks_api_key() is None - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_api_key_ini_empty_value(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - - mock_parser_instance.__contains__.side_effect = lambda item: item == "fireworks" - mock_parser_instance.has_option.side_effect = ( - lambda section, option: section == "fireworks" and option == "api_key" - ) - mock_parser_instance.get.side_effect = lambda section, option: ( - "" if section == "fireworks" and option == "api_key" else configparser.NoOptionError(option, section) - ) - - with patch("builtins.open", mock_open(read_data="[fireworks]\napi_key=")): - assert get_fireworks_api_key() is None - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_api_key_from_ini_default_section_success(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - - def has_option_logic(section, option): - if section == "fireworks": - return False - return section == mock_parser_instance.default_section and option == "api_key" - - def get_logic(section, option): - if section == mock_parser_instance.default_section and option == "api_key": - return INI_API_KEY - raise configparser.NoOptionError(option, section) - - mock_parser_instance.has_option.side_effect = has_option_logic - mock_parser_instance.get.side_effect = get_logic - mock_parser_instance.__contains__.side_effect = lambda item: item != "fireworks" - - with patch("builtins.open", mock_open(read_data="api_key = ini_api_key_abc")): - assert get_fireworks_api_key() == INI_API_KEY - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -@patch("pathlib.Path.exists", return_value=True) -# We don't mock ConfigParser here because we are testing the _parse_simple_auth_file path directly -def test_get_api_key_from_ini_simple_parsing_success(mock_path_exists): - file_content = f"api_key = {INI_API_KEY}\nother_key = value" - # Patch open specifically for _parse_simple_auth_file if it's different from configparser's use - with patch("eval_protocol.auth.open", mock_open(read_data=file_content), create=True): - # Ensure configparser path is not taken or also returns None for api_key - with patch("configparser.ConfigParser") as mock_ConfigParser_class_inner: - mock_parser_instance = mock_ConfigParser_class_inner.return_value - mock_parser_instance.read.return_value = [] # Simulate configparser finds nothing - mock_parser_instance.has_option.return_value = False - assert get_fireworks_api_key() == INI_API_KEY - - -# --- Tests for get_fireworks_account_id --- +def test_get_api_key_not_found(): + assert get_fireworks_api_key() is None def test_get_account_id_from_env(): @@ -207,190 +42,38 @@ def test_get_account_id_from_env(): assert get_fireworks_account_id() == TEST_ENV_ACCOUNT_ID -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) # Ensure simple parse finds nothing -def test_get_account_id_from_ini(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - - def has_option_fireworks_true(section, option): - if section == "fireworks": - return option == "account_id" - return False - - def get_fireworks_value(section, option): - if section == "fireworks" and option == "account_id": - return INI_ACCOUNT_ID - raise configparser.NoOptionError(option, section) - - mock_parser_instance.has_option.side_effect = has_option_fireworks_true - mock_parser_instance.get.side_effect = get_fireworks_value - mock_parser_instance.__contains__.side_effect = lambda item: item == "fireworks" - - with patch("builtins.open", mock_open(read_data="[fireworks]\naccount_id = foo")): - assert get_fireworks_account_id() == INI_ACCOUNT_ID - - mock_path_exists.assert_called_once_with() - mock_ConfigParser_class.assert_called_once_with() - mock_parser_instance.read.assert_called_once_with(AUTH_INI_FILE) - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -def test_get_account_id_env_overrides_ini(): - os.environ["FIREWORKS_ACCOUNT_ID"] = TEST_ENV_ACCOUNT_ID - with ( - patch("pathlib.Path.exists") as mock_path_exists, - patch("configparser.ConfigParser") as mock_ConfigParser_class, - patch("eval_protocol.auth._parse_simple_auth_file") as mock_parse_simple, - ): - assert get_fireworks_account_id() == TEST_ENV_ACCOUNT_ID - mock_parse_simple.assert_not_called() - mock_path_exists.assert_not_called() - mock_ConfigParser_class.assert_not_called() - - -@patch("pathlib.Path.exists", return_value=False) -def test_get_account_id_not_found(mock_path_exists): - with patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) as mock_parse_simple: - assert get_fireworks_account_id() is None - mock_parse_simple.assert_not_called() - # With verify fallback using get_fireworks_api_key, exists() may be checked more than once - assert mock_path_exists.call_count >= 1 - +def test_get_account_id_not_found(): + assert get_fireworks_account_id() is None -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_account_id_ini_exists_no_section(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.side_effect = configparser.MissingSectionHeaderError("file", 1, "line") - with patch( - "builtins.open", - mock_open(read_data="other_key = some_val_but_no_section_header\nanother=val"), - ): - assert get_fireworks_account_id() is None - # Fallback verify path may trigger a second simple parse for api_key; ensure at least one call - assert mock_parse_simple.call_count >= 1 - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_account_id_ini_exists_no_id_option(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - mock_parser_instance.__contains__.side_effect = lambda item: item == "fireworks" - mock_parser_instance.has_option.side_effect = lambda section, option: False - - with patch("builtins.open", mock_open(read_data="[fireworks]\nsome_other_key=foo")): - assert get_fireworks_account_id() is None - # Fallback verify path may trigger a second simple parse for api_key; ensure at least one call - assert mock_parse_simple.call_count >= 1 - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_account_id_ini_empty_value(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - mock_parser_instance.__contains__.side_effect = lambda item: item == "fireworks" - mock_parser_instance.has_option.side_effect = ( - lambda section, option: section == "fireworks" and option == "account_id" - ) - mock_parser_instance.get.side_effect = lambda section, option: ( - "" if section == "fireworks" and option == "account_id" else configparser.NoOptionError(option, section) - ) - with patch("builtins.open", mock_open(read_data="[fireworks]\naccount_id=")): - assert get_fireworks_account_id() is None - # Fallback verify path may trigger a second simple parse for api_key; ensure at least one call - assert mock_parse_simple.call_count >= 1 - - -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) -def test_get_account_id_from_ini_default_section_success(mock_parse_simple, mock_ConfigParser_class, mock_path_exists): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.return_value = [str(AUTH_INI_FILE)] - - def has_option_logic(section, option): - if section == "fireworks": - return False - return section == mock_parser_instance.default_section and option == "account_id" - - def get_logic(section, option): - if section == mock_parser_instance.default_section and option == "account_id": - return INI_ACCOUNT_ID - raise configparser.NoOptionError(option, section) - - mock_parser_instance.has_option.side_effect = has_option_logic - mock_parser_instance.get.side_effect = get_logic - mock_parser_instance.__contains__.side_effect = lambda item: item != "fireworks" - with patch("builtins.open", mock_open(read_data="account_id = ini_account_id_def")): - assert get_fireworks_account_id() == INI_ACCOUNT_ID - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) - - -@patch("pathlib.Path.exists", return_value=True) -# We don't mock ConfigParser here because we are testing the _parse_simple_auth_file path directly -def test_get_account_id_from_ini_simple_parsing_success( - mock_path_exists, -): # Renamed from fallback_parsing - file_content = f"account_id = {INI_ACCOUNT_ID}\nother_key = value" - with patch("eval_protocol.auth.open", mock_open(read_data=file_content), create=True): - # Ensure configparser path is not taken or also returns None for account_id - with patch("configparser.ConfigParser") as mock_ConfigParser_class_inner: - mock_parser_instance = mock_ConfigParser_class_inner.return_value - mock_parser_instance.read.return_value = [] - mock_parser_instance.has_option.return_value = False - assert get_fireworks_account_id() == INI_ACCOUNT_ID +def test_verify_api_key_and_get_account_id_success_from_header(): + os.environ["FIREWORKS_API_KEY"] = TEST_ENV_API_KEY -# --- Tests for error handling --- + class _Resp: + status_code = 200 + headers = {"x-fireworks-account-id": "acct_123"} + with patch("eval_protocol.auth.requests.get", return_value=_Resp()): + assert verify_api_key_and_get_account_id() == "acct_123" -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) # Simple parse finds nothing -def test_get_api_key_ini_parse_error(mock_parse_simple, mock_ConfigParser_class, mock_path_exists, caplog): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.side_effect = configparser.Error("Mocked Parsing Error") - with patch("builtins.open", mock_open(read_data="malformed ini content")): - assert get_fireworks_api_key() is None - assert "Configparser error reading" in caplog.text - assert "Mocked Parsing Error" in caplog.text - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) +def test_verify_api_key_and_get_account_id_non_200_returns_none(): + os.environ["FIREWORKS_API_KEY"] = TEST_ENV_API_KEY + class _Resp: + status_code = 403 + headers = {"x-fireworks-account-id": "acct_789"} -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) # Simple parse finds nothing -def test_get_account_id_ini_parse_error(mock_parse_simple, mock_ConfigParser_class, mock_path_exists, caplog): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.side_effect = configparser.Error("Mocked Parsing Error") + with patch("eval_protocol.auth.requests.get", return_value=_Resp()): + assert verify_api_key_and_get_account_id() is None - with patch("builtins.open", mock_open(read_data="malformed ini content")): - assert get_fireworks_account_id() is None - assert "Configparser error reading" in caplog.text - assert "Mocked Parsing Error" in caplog.text - # Fallback verify path may trigger a second simple parse for api_key; ensure at least one call - assert mock_parse_simple.call_count >= 1 +def test_get_account_id_resolves_via_verify_when_api_key_present(): + os.environ["FIREWORKS_API_KEY"] = TEST_ENV_API_KEY -@patch("pathlib.Path.exists", return_value=True) -@patch("configparser.ConfigParser") -@patch("eval_protocol.auth._parse_simple_auth_file", return_value={}) # Simple parse finds nothing -def test_get_api_key_unexpected_error_reading_ini( - mock_parse_simple, mock_ConfigParser_class, mock_path_exists, caplog -): - mock_parser_instance = mock_ConfigParser_class.return_value - mock_parser_instance.read.side_effect = Exception("Unexpected Read Error") + class _Resp: + status_code = 200 + headers = {"X-Fireworks-Account-Id": "acct_456"} - with patch("builtins.open", mock_open(read_data="ini content")): - assert get_fireworks_api_key() is None - assert "Unexpected error reading" in caplog.text # This comes from _get_credential_from_config_file - assert "Unexpected Read Error" in caplog.text - mock_parse_simple.assert_called_once_with(AUTH_INI_FILE) + with patch("eval_protocol.auth.requests.get", return_value=_Resp()): + assert get_fireworks_account_id() == "acct_456" From ad1ee4ad0994f7665da0f2b335a6c662fb968fd1 Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Tue, 16 Dec 2025 12:16:16 -0800 Subject: [PATCH 2/2] addres comments --- .env.example | 1 - development/CONTRIBUTING.md | 14 +++----------- development/CORE_STRATEGY.md | 4 ++-- development/record_replay_testing_handoff.md | 1 - docs/cli_reference/cli_overview.mdx | 1 - eval_protocol/auth.py | 19 +++++-------------- eval_protocol/cli_commands/create_rft.py | 2 +- eval_protocol/cli_commands/utils.py | 13 ++----------- eval_protocol/platform_api.py | 5 ++--- .../metrics/llm_resource_example/README.md | 3 +-- examples/taxi_mcp_complete/README.md | 1 - tests/test_auth.py | 8 +------- tests/test_cli_create_rft.py | 10 ++++++---- tests/test_deploy_integration.py | 5 +++-- tests/test_ep_upload_e2e.py | 7 ++++++- tests/test_evaluation.py | 6 +++--- tests/test_evaluation_integration.py | 2 +- tests/test_evaluation_preview_integration.py | 2 +- tests/test_examples_end_to_end.py | 2 +- 19 files changed, 38 insertions(+), 68 deletions(-) diff --git a/.env.example b/.env.example index 41810d09..584bb4f9 100644 --- a/.env.example +++ b/.env.example @@ -5,7 +5,6 @@ # Fireworks AI Credentials (for interacting with the Fireworks platform) # These are used by eval protocol to deploy evaluators to Fireworks, preview, etc. FIREWORKS_API_KEY="your_fireworks_api_key_here" -FIREWORKS_ACCOUNT_ID="your_fireworks_account_id_here" # e.g., "fireworks" or your specific account # Optional: If targeting a non-production Fireworks API endpoint # FIREWORKS_API_BASE="https://dev.api.fireworks.ai" diff --git a/development/CONTRIBUTING.md b/development/CONTRIBUTING.md index d715fb69..6d9343a2 100644 --- a/development/CONTRIBUTING.md +++ b/development/CONTRIBUTING.md @@ -66,12 +66,11 @@ For a streamlined local development experience, especially when managing multipl cp .env.example .env.dev ``` 2. **Populate `.env.dev`:** - Open `.env.dev` and fill in the necessary environment variables, such as `FIREWORKS_API_KEY`, `FIREWORKS_ACCOUNT_ID`, and any other variables required for your development tasks (e.g., `E2B_API_KEY`). + Open `.env.dev` and fill in the necessary environment variables, such as `FIREWORKS_API_KEY` and any other variables required for your development tasks (e.g., `E2B_API_KEY`). Example content for `.env.dev`: ``` FIREWORKS_API_KEY="your_dev_fireworks_api_key" - FIREWORKS_ACCOUNT_ID="abc" FIREWORKS_API_BASE="https://api.fireworks.ai" E2B_API_KEY="your_e2b_api_key" ``` @@ -92,15 +91,12 @@ Set the following environment variables. For development, you might use specific * `FIREWORKS_API_KEY`: Your Fireworks AI API key. * For development, you might use a specific dev key: `export FIREWORKS_API_KEY="your_dev_fireworks_api_key"` -* `FIREWORKS_ACCOUNT_ID`: Your Fireworks AI Account ID. This is used to scope operations to your account. - * For development against a shared dev environment, this might be a common ID like `pyroworks-dev`: `export FIREWORKS_ACCOUNT_ID="pyroworks-dev"` * `FIREWORKS_API_BASE`: (Optional) If you need to target a non-production Fireworks API endpoint. * For development: `export FIREWORKS_API_BASE="https://dev.api.fireworks.ai"` Example for a typical development setup: ```bash export FIREWORKS_API_KEY="your_development_api_key" -export FIREWORKS_ACCOUNT_ID="pyroworks-dev" # Or your specific dev account ID export FIREWORKS_API_BASE="https://dev.api.fireworks.ai" # If targeting dev API ``` @@ -110,11 +106,10 @@ Eval Protocol does not read `~/.fireworks/auth.ini` (or any firectl profiles). U **Credential Sourcing Order:** Eval Protocol prioritizes credentials as follows: -1. Environment Variables (`FIREWORKS_API_KEY`, optional `FIREWORKS_ACCOUNT_ID`) +1. Environment Variables (`FIREWORKS_API_KEY`) **Purpose of Credentials:** * `FIREWORKS_API_KEY`: Authenticates your requests to the Fireworks AI service. -* `FIREWORKS_ACCOUNT_ID`: Identifies your account for operations like managing evaluators. Typically this is derived automatically from `FIREWORKS_API_KEY` via the `verifyApiKey` endpoint. * `FIREWORKS_API_BASE`: Allows targeting different API environments (e.g., development, staging). **Other Environment Variables:** @@ -490,11 +485,8 @@ This is perfect for development, webhook testing, or making your reward function If you encounter authentication issues: 1. **Check Credential Sources**: - * Verify that `FIREWORKS_API_KEY` and `FIREWORKS_ACCOUNT_ID` are correctly set as environment variables. - * If not using environment variables, ensure `~/.fireworks/auth.ini` exists, is correctly formatted, and contains the right `api_key` and `account_id` under the `[fireworks]` section. - * Remember the priority: environment variables override the `auth.ini` file. + * Verify that `FIREWORKS_API_KEY` is correctly set as an environment variable. 2. **Verify API Key Permissions**: Ensure the API key has the necessary permissions for the operations you are attempting. -3. **Check Account ID**: Confirm that the `FIREWORKS_ACCOUNT_ID` is correct for the environment you are targeting (e.g., `pyroworks-dev` for the dev API, or your personal account ID). 4. **API Base URL**: If using `FIREWORKS_API_BASE`, ensure it points to the correct API endpoint (e.g., `https://dev.api.fireworks.ai` for development). You can use the following snippet to check what credentials Eval Protocol is resolving: diff --git a/development/CORE_STRATEGY.md b/development/CORE_STRATEGY.md index e0249f31..1d4ed3a3 100644 --- a/development/CORE_STRATEGY.md +++ b/development/CORE_STRATEGY.md @@ -52,8 +52,8 @@ This roadmap outlines the development priorities. Tasks within each phase can be * [ ] **Docstring Checking:** Enable and enforce docstring checking. * **P1.5: Authentication System Refactor:** * [ ] **Centralize Logic:** Create `eval_protocol/auth.py`. - * [ ] **Configuration Methods:** Support environment variables and `auth.ini`. - * [ ] **Documentation:** Clearly document `FIREWORKS_ACCOUNT_ID` usage and auth setup. + * [ ] **Configuration Methods:** Support `FIREWORKS_API_KEY` (environment variable) as the single source of truth. + * [ ] **Documentation:** Clearly document API-key-based auth and account id derivation. * [ ] **Codebase Update:** Refactor `eval_protocol/evaluation.py` etc. to use new auth module. * **P1.6: Build, Packaging, and Basic CI:** * [ ] **`setup.py` Review:** Evaluate `openai` pinning, clean `extras_require`, populate `long_description`. diff --git a/development/record_replay_testing_handoff.md b/development/record_replay_testing_handoff.md index c0939023..668759ea 100644 --- a/development/record_replay_testing_handoff.md +++ b/development/record_replay_testing_handoff.md @@ -101,7 +101,6 @@ PORT=8001 python simulation_server.py --port 8001 ### Environment Variables ```bash export FIREWORKS_API_KEY="your_dev_fireworks_api_key" -export FIREWORKS_ACCOUNT_ID="your_account_id" export EP_PLAYBACK_FILE="/path/to/recording.jsonl" # For playback mode ``` diff --git a/docs/cli_reference/cli_overview.mdx b/docs/cli_reference/cli_overview.mdx index e9303d2f..9ee7b098 100644 --- a/docs/cli_reference/cli_overview.mdx +++ b/docs/cli_reference/cli_overview.mdx @@ -329,7 +329,6 @@ The CLI recognizes the following environment variables: - `FIREWORKS_API_KEY`: Your Fireworks API key (required for deployment operations) - `FIREWORKS_API_BASE`: Base URL for the Fireworks API (defaults to `https://api.fireworks.ai`) -- `FIREWORKS_ACCOUNT_ID`: Your Fireworks account ID (optional; typically derived automatically from `FIREWORKS_API_KEY`) - `MODEL_AGENT`: Default agent model to use (e.g., "openai/gpt-4o-mini") - `MODEL_SIM`: Default simulation model to use (e.g., "openai/gpt-3.5-turbo") diff --git a/eval_protocol/auth.py b/eval_protocol/auth.py index 27781468..68ce134c 100644 --- a/eval_protocol/auth.py +++ b/eval_protocol/auth.py @@ -26,30 +26,21 @@ def get_fireworks_account_id() -> Optional[str]: """ Retrieves the Fireworks Account ID. - The Account ID is sourced in the following order: - 1. FIREWORKS_ACCOUNT_ID environment variable. - 2. If an API key is available (env), resolve via verifyApiKey. - Returns: The Account ID if found, otherwise None. """ - account_id = os.environ.get("FIREWORKS_ACCOUNT_ID") - if account_id and account_id.strip(): - logger.debug("Using FIREWORKS_ACCOUNT_ID from environment variable.") - return account_id.strip() - - # Fallback: if API key is present, attempt to resolve via verifyApiKey (env) + # Account id is derived from the API key (single source of truth). try: api_key_for_verify = get_fireworks_api_key() if api_key_for_verify: resolved = verify_api_key_and_get_account_id(api_key=api_key_for_verify, api_base=get_fireworks_api_base()) if resolved: - logger.debug("Using FIREWORKS_ACCOUNT_ID resolved via verifyApiKey: %s", resolved) + logger.debug("Resolved account id via verifyApiKey: %s", resolved) return resolved except Exception as e: - logger.debug("Failed to resolve FIREWORKS_ACCOUNT_ID via verifyApiKey: %s", e) + logger.debug("Failed to resolve account id via verifyApiKey: %s", e) - logger.debug("Fireworks Account ID not found in environment variables or via verifyApiKey.") + logger.debug("Fireworks Account ID not found via verifyApiKey.") return None @@ -107,7 +98,7 @@ def verify_api_key_and_get_account_id( # Header keys could vary in case; requests provides case-insensitive dict account_id = resp.headers.get("x-fireworks-account-id") or resp.headers.get("X-Fireworks-Account-Id") if account_id and account_id.strip(): - logger.debug("Resolved FIREWORKS_ACCOUNT_ID via verifyApiKey: %s", account_id) + logger.debug("Resolved account id via verifyApiKey: %s", account_id) return account_id.strip() return None except Exception as e: diff --git a/eval_protocol/cli_commands/create_rft.py b/eval_protocol/cli_commands/create_rft.py index c490f374..7f0a4769 100644 --- a/eval_protocol/cli_commands/create_rft.py +++ b/eval_protocol/cli_commands/create_rft.py @@ -753,7 +753,7 @@ def create_rft_command(args) -> int: account_id = _ensure_account_id() if not account_id: - print("Error: FIREWORKS_ACCOUNT_ID not set and could not be resolved.") + print("Error: Could not resolve Fireworks account id from FIREWORKS_API_KEY.") return 1 api_base = get_fireworks_api_base() diff --git a/eval_protocol/cli_commands/utils.py b/eval_protocol/cli_commands/utils.py index d49b97e9..9cec1722 100644 --- a/eval_protocol/cli_commands/utils.py +++ b/eval_protocol/cli_commands/utils.py @@ -396,20 +396,11 @@ def _normalize_evaluator_id(evaluator_id: str) -> str: def _ensure_account_id() -> Optional[str]: - """Resolve and cache FIREWORKS_ACCOUNT_ID if possible.""" - account_id = os.environ.get("FIREWORKS_ACCOUNT_ID") - if account_id and account_id.strip(): - return account_id.strip() - + """Resolve Fireworks account id from FIREWORKS_API_KEY via verifyApiKey.""" api_key = get_fireworks_api_key() if not api_key: return None - - resolved = verify_api_key_and_get_account_id(api_key=api_key, api_base=get_fireworks_api_base()) - if resolved: - os.environ["FIREWORKS_ACCOUNT_ID"] = resolved - return resolved - return None + return verify_api_key_and_get_account_id(api_key=api_key, api_base=get_fireworks_api_base()) def _extract_terminal_segment(resource_name: str) -> str: diff --git a/eval_protocol/platform_api.py b/eval_protocol/platform_api.py index 81754e13..bf608be0 100644 --- a/eval_protocol/platform_api.py +++ b/eval_protocol/platform_api.py @@ -302,7 +302,6 @@ def delete_fireworks_secret( # These should be set in your .env.dev, .env file (or shell environment) for this test to run # FIREWORKS_API_KEY="your_fireworks_api_key" - # FIREWORKS_ACCOUNT_ID="your_fireworks_account_id" # FIREWORKS_API_BASE="https://api.fireworks.ai" # or your dev/staging endpoint test_account_id = get_fireworks_account_id() @@ -310,7 +309,7 @@ def delete_fireworks_secret( test_api_base = get_fireworks_api_base() logger.info("Attempting to use the following configuration for testing Fireworks secrets API:") - logger.info(f" Resolved FIREWORKS_ACCOUNT_ID: {test_account_id}") + logger.info(f" Resolved account id (derived from API key): {test_account_id}") logger.info(f" Resolved FIREWORKS_API_BASE: {test_api_base}") logger.info( f" Resolved FIREWORKS_API_KEY: {'********' + test_api_key[-4:] if test_api_key and len(test_api_key) > 4 else 'Not set or too short'}" @@ -318,7 +317,7 @@ def delete_fireworks_secret( if not test_account_id or not test_api_key or not test_api_base: logger.error( - "CRITICAL: FIREWORKS_ACCOUNT_ID, FIREWORKS_API_KEY, and FIREWORKS_API_BASE must be correctly set in environment or .env file to run this test." + "CRITICAL: FIREWORKS_API_KEY and FIREWORKS_API_BASE must be correctly set in environment or .env file to run this test." ) import sys # Make sure sys is imported if using sys.exit diff --git a/examples/metrics/llm_resource_example/README.md b/examples/metrics/llm_resource_example/README.md index 8cb6f83c..57de23d3 100644 --- a/examples/metrics/llm_resource_example/README.md +++ b/examples/metrics/llm_resource_example/README.md @@ -21,14 +21,13 @@ This example demonstrates how to use an LLM as a judge within the reward functio ```bash # Set your Fireworks API key export FIREWORKS_API_KEY=your_api_key_here -export FIREWORKS_ACCOUNT_ID=pyroworks ``` ## Running the Example ```bash # Run evaluation using the configuration file -FIREWORKS_ACCOUNT_ID=pyroworks eval-protocol run --config-path conf --config-name simple_llm_judge_eval +eval-protocol run --config-path conf --config-name simple_llm_judge_eval ``` ## Configuration diff --git a/examples/taxi_mcp_complete/README.md b/examples/taxi_mcp_complete/README.md index 3aa36969..9d26e7d0 100644 --- a/examples/taxi_mcp_complete/README.md +++ b/examples/taxi_mcp_complete/README.md @@ -94,7 +94,6 @@ source .venv/bin/activate # Set up authentication export FIREWORKS_API_KEY="your_dev_fireworks_api_key" -export FIREWORKS_ACCOUNT_ID="your_account_id" ``` ### Proper Testing (Recommended) diff --git a/tests/test_auth.py b/tests/test_auth.py index ef280557..047f85a7 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -11,12 +11,11 @@ TEST_ENV_API_KEY = "test_env_api_key_123" -TEST_ENV_ACCOUNT_ID = "test_env_account_id_456" @pytest.fixture(autouse=True) def clear_env_vars_fixture(): - env_vars_to_clear = ["FIREWORKS_API_KEY", "FIREWORKS_ACCOUNT_ID", "FIREWORKS_API_BASE"] + env_vars_to_clear = ["FIREWORKS_API_KEY", "FIREWORKS_API_BASE"] original_values = {var: os.environ.get(var) for var in env_vars_to_clear} for var in env_vars_to_clear: os.environ.pop(var, None) @@ -37,11 +36,6 @@ def test_get_api_key_not_found(): assert get_fireworks_api_key() is None -def test_get_account_id_from_env(): - os.environ["FIREWORKS_ACCOUNT_ID"] = TEST_ENV_ACCOUNT_ID - assert get_fireworks_account_id() == TEST_ENV_ACCOUNT_ID - - def test_get_account_id_not_found(): assert get_fireworks_account_id() is None diff --git a/tests/test_cli_create_rft.py b/tests/test_cli_create_rft.py index ec6732de..97f07cb1 100644 --- a/tests/test_cli_create_rft.py +++ b/tests/test_cli_create_rft.py @@ -10,6 +10,7 @@ from eval_protocol.cli_commands import upload as upload_mod import eval_protocol.fireworks_rft as fr from eval_protocol.cli import parse_args +import eval_protocol.cli_commands.utils as cli_utils def _write_json(path: str, data: dict) -> None: @@ -34,8 +35,9 @@ def rft_test_harness(tmp_path, monkeypatch): # Environment required by command monkeypatch.setenv("FIREWORKS_API_KEY", "fw_dummy") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "acct123") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + # Account id is derived from API key; mock the verify call to keep tests offline. + monkeypatch.setattr(cli_utils, "verify_api_key_and_get_account_id", lambda *a, **k: "acct123") monkeypatch.setattr(upload_mod, "_prompt_select", lambda tests, non_interactive=False: tests[:1]) monkeypatch.setattr(upload_mod, "upload_command", lambda args: 0) @@ -634,8 +636,8 @@ def test_create_rft_quiet_existing_evaluator_skips_upload(tmp_path, monkeypatch) # Env monkeypatch.setenv("FIREWORKS_API_KEY", "fw_dummy") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "acct123") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr(cli_utils, "verify_api_key_and_get_account_id", lambda *a, **k: "acct123") # Mock evaluator exists and is ACTIVE class _Resp: @@ -697,8 +699,8 @@ def test_create_rft_quiet_new_evaluator_ambiguous_without_entry_errors(tmp_path, # Env monkeypatch.setenv("FIREWORKS_API_KEY", "fw_dummy") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "acct123") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr(cli_utils, "verify_api_key_and_get_account_id", lambda *a, **k: "acct123") # Evaluator does not exist (force path into upload section) def _raise(*a, **k): @@ -1039,8 +1041,8 @@ def test_cli_full_command_style_evaluator_and_dataset_flags(tmp_path, monkeypatc # Env monkeypatch.setenv("FIREWORKS_API_KEY", "fw_dummy") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "pyroworks-dev") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr(cli_utils, "verify_api_key_and_get_account_id", lambda *a, **k: "pyroworks-dev") # Mock evaluator exists and ACTIVE class _Resp: diff --git a/tests/test_deploy_integration.py b/tests/test_deploy_integration.py index 6b7db1b0..1841c9db 100644 --- a/tests/test_deploy_integration.py +++ b/tests/test_deploy_integration.py @@ -81,8 +81,9 @@ def deploy_example(): def mock_env_variables(monkeypatch): """Set environment variables for testing""" monkeypatch.setenv("FIREWORKS_API_KEY", "test_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_account") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + # Account id is derived from API key; mock deploy module lookup to keep tests offline. + monkeypatch.setattr("eval_protocol.cli_commands.deploy.get_fireworks_account_id", lambda: "test_account") @pytest.fixture @@ -109,7 +110,7 @@ def mock_requests_get(): def test_deploy_gcp_with_inline_requirements( - mock_env_variables, # Ensures FIREWORKS_ACCOUNT_ID etc. are set + mock_env_variables, # Ensures FIREWORKS_API_KEY etc. are set create_dummy_reward_module_for_deploy, # Creates and cleans up the dummy module ): """ diff --git a/tests/test_ep_upload_e2e.py b/tests/test_ep_upload_e2e.py index a1eba1d4..a1521a96 100644 --- a/tests/test_ep_upload_e2e.py +++ b/tests/test_ep_upload_e2e.py @@ -19,6 +19,8 @@ import pytest +import eval_protocol.cli_commands.utils as cli_utils + def create_test_project_with_evaluation_test(test_content: str, filename: str = "test_eval.py"): """ @@ -52,9 +54,12 @@ def create_test_project_with_evaluation_test(test_content: str, filename: str = def mock_env_variables(monkeypatch): """Set up test environment variables""" monkeypatch.setenv("FIREWORKS_API_KEY", "test_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_account") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr(cli_utils, "verify_api_key_and_get_account_id", lambda *a, **k: "test_account") + # Upload ultimately calls into eval_protocol.evaluation for API calls; keep it offline too. + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_account") + @pytest.fixture def mock_requests_get(): diff --git a/tests/test_evaluation.py b/tests/test_evaluation.py index 7c4ab808..e251d6d9 100644 --- a/tests/test_evaluation.py +++ b/tests/test_evaluation.py @@ -169,7 +169,7 @@ def test_evaluator_preview(mock_requests_post, monkeypatch): mock_requests_post.return_value = mock_response monkeypatch.setenv("FIREWORKS_API_KEY", "test_preview_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_preview_account") + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_preview_account") # Using a mock API base to prevent real calls monkeypatch.setenv("FIREWORKS_API_BASE", "http://mock-api-server") # Changed to avoid actual localhost call @@ -269,7 +269,7 @@ def test_preview_evaluation_helper(mock_requests_post, monkeypatch): mock_requests_post.return_value = mock_response monkeypatch.setenv("FIREWORKS_API_KEY", "test_helper_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_helper_account") + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_helper_account") # Using a mock API base to prevent real calls monkeypatch.setenv("FIREWORKS_API_BASE", "http://mock-api-server-helper") # Changed @@ -333,7 +333,7 @@ def mock_post_helper_preview(*args, **kwargs): def test_create_evaluation_helper(monkeypatch): tmp_dir = create_test_folder() monkeypatch.setenv("FIREWORKS_API_KEY", "test_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_account") + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_account") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") original_cwd = os.getcwd() diff --git a/tests/test_evaluation_integration.py b/tests/test_evaluation_integration.py index 003dee30..de97000b 100644 --- a/tests/test_evaluation_integration.py +++ b/tests/test_evaluation_integration.py @@ -72,8 +72,8 @@ def create_sample_file(): @pytest.fixture def mock_env_variables(monkeypatch): monkeypatch.setenv("FIREWORKS_API_KEY", "test_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_account") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_account") @pytest.fixture diff --git a/tests/test_evaluation_preview_integration.py b/tests/test_evaluation_preview_integration.py index fe45ca0c..d7b3b266 100644 --- a/tests/test_evaluation_preview_integration.py +++ b/tests/test_evaluation_preview_integration.py @@ -38,8 +38,8 @@ def evaluation_preview_example(): def mock_env_variables(monkeypatch): """Set environment variables for testing""" monkeypatch.setenv("FIREWORKS_API_KEY", "test_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_account") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_account") @pytest.fixture diff --git a/tests/test_examples_end_to_end.py b/tests/test_examples_end_to_end.py index fdb35496..bd0484a4 100644 --- a/tests/test_examples_end_to_end.py +++ b/tests/test_examples_end_to_end.py @@ -35,8 +35,8 @@ def examples_path(): def mock_env_variables(monkeypatch): """Set environment variables for testing""" monkeypatch.setenv("FIREWORKS_API_KEY", "test_api_key") - monkeypatch.setenv("FIREWORKS_ACCOUNT_ID", "test_account") monkeypatch.setenv("FIREWORKS_API_BASE", "https://api.fireworks.ai") + monkeypatch.setattr("eval_protocol.evaluation.get_fireworks_account_id", lambda: "test_account") @pytest.fixture