Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/k8s-multinode-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ jobs:

- name: Run k8s multinode E2E test
env:
PLUTO_API_TOKEN: ${{ secrets.MLOP_API_TOKEN }}
PLUTO_API_KEY: ${{ secrets.MLOP_API_TOKEN }}
PLUTO_DEBUG_LEVEL: DEBUG
run: |
set -euox pipefail
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/neptune-compat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ jobs:

- name: Setup pluto credentials for dual-logging tests
env:
PLUTO_API_TOKEN: ${{ secrets.MLOP_API_TOKEN }}
PLUTO_API_KEY: ${{ secrets.MLOP_API_TOKEN }}
run: |
set -euox pipefail
poetry run pluto login "${PLUTO_API_TOKEN}"
poetry run pluto login "${PLUTO_API_KEY}"

- name: Run Neptune dual-logging integration tests
env:
Expand Down
4 changes: 2 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ Settings can be provided via:
Environment variables use the `PLUTO_*` prefix. The old `MLOP_*` prefix is supported with deprecation warnings.

**Authentication & Project:**
- `PLUTO_API_TOKEN` - API token for authentication (alternative to `pluto login`)
- `PLUTO_API_KEY` - API token for authentication (alternative to `pluto login`)
- `PLUTO_PROJECT` - Default project name (alternative to `pluto.init(project="...")`)

**Configuration:**
Expand All @@ -303,7 +303,7 @@ Environment variables use the `PLUTO_*` prefix. The old `MLOP_*` prefix is suppo

### Testing Notes
- Tests run against production server by default
- Requires authentication via `PLUTO_API_TOKEN` environment variable
- Requires authentication via `PLUTO_API_KEY` environment variable
- Tests marked with `@pytest.mark.distributed` require multi-rank torch setup
- Use `HAS_TORCH`, `HAS_MATPLOTLIB` flags for optional dependency tests

Expand Down
2 changes: 1 addition & 1 deletion docs/SYNC_PROCESS_V2_ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ $ pluto finish my-experiment-20240115 --force
```python
# Environment variables (for containerized training)
PLUTO_PROJECT=my-project
PLUTO_API_TOKEN=xxx
PLUTO_API_KEY=xxx
PLUTO_RUN_DIR=/tmp/pluto-runs # Where to store local data
PLUTO_SYNC_MODE=process # process | thread | offline

Expand Down
10 changes: 5 additions & 5 deletions pluto/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import pluto.query as pq

# Uses PLUTO_API_TOKEN env var and default server
# Uses PLUTO_API_KEY env var and default server
runs = pq.list_runs("my-project")
run = pq.get_run("my-project", "MMP-42")
metrics = pq.get_metrics("my-project", run["id"], metric_names=["val/loss"])
Expand Down Expand Up @@ -49,7 +49,7 @@ class Client:
"""HTTP client for reading data from the Pluto server.

Args:
api_token: API token for authentication. Defaults to ``PLUTO_API_TOKEN``
api_token: API token for authentication. Defaults to ``PLUTO_API_KEY``
environment variable.
host: Server URL (e.g. ``https://pluto.trainy.ai`` or
``https://pluto-api.trainy.ai``). When a bare hostname or
Expand All @@ -68,7 +68,7 @@ def __init__(
self._api_token = api_token or _resolve_api_token()
if not self._api_token:
raise QueryError(
'No API token provided. Set PLUTO_API_TOKEN environment variable '
'No API token provided. Set PLUTO_API_KEY environment variable '
'or pass api_token to Client().'
)

Expand Down Expand Up @@ -637,15 +637,15 @@ def get_logs(

def _resolve_api_token() -> Optional[str]:
"""Resolve API token from environment or keyring."""
token = os.environ.get('PLUTO_API_TOKEN')
token = os.environ.get('PLUTO_API_KEY')

Choose a reason for hiding this comment

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

high

This change removes support for PLUTO_API_TOKEN entirely, which is a breaking change. To provide a smoother transition for users, consider maintaining backward compatibility by supporting PLUTO_API_TOKEN with a deprecation warning, similar to how MLOP_API_TOKEN is handled.

The recommended resolution order would be:

  1. PLUTO_API_KEY
  2. PLUTO_API_TOKEN (with deprecation warning)
  3. MLOP_API_TOKEN (with deprecation warning)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gemini-code-assist PLUTO_API_KEY was there before (in pluto/compat/neptune.py) so it shouldn't break anything

Choose a reason for hiding this comment

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

Thanks for the clarification, Nicoowr. I understand that PLUTO_API_KEY might have been present or used in other parts of the codebase, such as pluto/compat/neptune.py. However, the concern here is specifically about the _resolve_api_token function in pluto/query.py.

Before this change, _resolve_api_token explicitly looked for PLUTO_API_TOKEN as its primary environment variable. By changing line 640 from os.environ.get('PLUTO_API_TOKEN') to os.environ.get('PLUTO_API_KEY') and not adding PLUTO_API_TOKEN as a fallback, any user who has PLUTO_API_TOKEN set but not PLUTO_API_KEY will find that their API token is no longer resolved by this function. This constitutes a breaking change for those users.

To maintain backward compatibility and provide a smoother transition, I recommend modifying the _resolve_api_token function to check for PLUTO_API_TOKEN after PLUTO_API_KEY, but with a deprecation warning. This aligns with the approach already taken for MLOP_API_TOKEN.

The revised resolution order for _resolve_api_token should be:

  1. Check for PLUTO_API_KEY.
  2. If not found, check for PLUTO_API_TOKEN and issue a deprecation warning.
  3. If not found, check for MLOP_API_TOKEN and issue a deprecation warning.

Here's a conceptual example of how the _resolve_api_token function could be structured to achieve this:

def _resolve_api_token() -> Optional[str]:
    """Resolve API token from environment or keyring."""
    token = os.environ.get('PLUTO_API_KEY')
    if token:
        return token

    token = os.environ.get('PLUTO_API_TOKEN')
    if token:
        warnings.warn(
            'PLUTO_API_TOKEN is deprecated. Use PLUTO_API_KEY instead.',
            DeprecationWarning,
            stacklevel=3,
        )
        return token

    token = os.environ.get('MLOP_API_TOKEN')
    if token:
        warnings.warn(
            'MLOP_API_TOKEN is deprecated. Use PLUTO_API_KEY instead.',
            DeprecationWarning,
            stacklevel=3,
        )
        return token

    # ... (keyring lookup logic)
    return None

if token:
return token

# Deprecated env var
token = os.environ.get('MLOP_API_TOKEN')
if token:
warnings.warn(
'MLOP_API_TOKEN is deprecated. Use PLUTO_API_TOKEN instead.',
'MLOP_API_TOKEN is deprecated. Use PLUTO_API_KEY instead.',
DeprecationWarning,
stacklevel=3,
)
Expand Down
4 changes: 2 additions & 2 deletions pluto/sets.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,9 @@ def setup(settings: Union[Settings, Dict[str, Any], None] = None) -> Settings:
env_value if env_value is not None else default_value
)

# Read PLUTO_API_TOKEN environment variable (with MLOP_API_TOKEN fallback)
# Read PLUTO_API_KEY environment variable (with MLOP_API_TOKEN fallback)
# Only apply if not already set via function parameters
env_api_token = _get_env_with_deprecation('PLUTO_API_TOKEN', 'MLOP_API_TOKEN')
env_api_token = _get_env_with_deprecation('PLUTO_API_KEY', 'MLOP_API_TOKEN')

Choose a reason for hiding this comment

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

high

Similar to my comment in pluto/query.py, this logic removes support for PLUTO_API_TOKEN, which is a breaking change. For backward compatibility, please consider supporting PLUTO_API_TOKEN with a deprecation warning.

The recommended resolution order is PLUTO_API_KEY, then PLUTO_API_TOKEN, then MLOP_API_TOKEN.

if env_api_token is not None and '_auth' not in settings_dict:
new_settings._auth = env_api_token

Expand Down
2 changes: 1 addition & 1 deletion tests/k8s/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Verifies that multiple nodes in a konduktor job can log to the same Pluto run vi
## Running

```bash
export PLUTO_API_TOKEN="your-token"
export PLUTO_API_KEY="your-token"
./tests/k8s/run_multinode_test.sh
```

Expand Down
6 changes: 3 additions & 3 deletions tests/k8s/multinode_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

Environment variables (set by konduktor):
- PLUTO_RUN_ID: Shared run ID for all nodes
- PLUTO_API_TOKEN: API token for authentication
- PLUTO_API_KEY: API token for authentication
- MASTER_ADDR: Address of the master node
- NUM_NODES: Total number of nodes
- RANK: Global rank of this node (0, 1, 2, ...)
Expand Down Expand Up @@ -47,8 +47,8 @@ def main():
print(f'[Node {rank}] ERROR: PLUTO_RUN_ID not set!')
sys.exit(1)

if not os.environ.get('PLUTO_API_TOKEN'):
print(f'[Node {rank}] ERROR: PLUTO_API_TOKEN not set!')
if not os.environ.get('PLUTO_API_KEY'):
print(f'[Node {rank}] ERROR: PLUTO_API_KEY not set!')
sys.exit(1)

# Initialize Pluto with shared run_id
Expand Down
6 changes: 3 additions & 3 deletions tests/k8s/run_multinode_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#
# Prerequisites:
# - docker, kind, kubectl
# - PLUTO_API_TOKEN environment variable
# - PLUTO_API_KEY environment variable
#
# Options:
# - KEEP_CLUSTER=true to keep cluster after test
Expand Down Expand Up @@ -83,7 +83,7 @@ check_prerequisites() {
command -v docker &>/dev/null || error "docker not installed"
command -v kind &>/dev/null || error "kind not installed"
command -v kubectl &>/dev/null || error "kubectl not installed"
[[ -n "${PLUTO_API_TOKEN:-}" ]] || error "PLUTO_API_TOKEN not set"
[[ -n "${PLUTO_API_KEY:-}" ]] || error "PLUTO_API_KEY not set"
log "Prerequisites OK"
}

Expand Down Expand Up @@ -150,7 +150,7 @@ run_test() {
log "Launching job with konduktor..."
konduktor launch --name "${JOB_NAME}" -y "${SCRIPT_DIR}/multinode-job.yaml" \
--env "PLUTO_RUN_ID=${run_id}" \
--env "PLUTO_API_TOKEN=${PLUTO_API_TOKEN}" \
--env "PLUTO_API_KEY=${PLUTO_API_KEY}" \
--env "PLUTO_PROJECT=testing-ci"

log "Job launched. Checking initial status..."
Expand Down
16 changes: 8 additions & 8 deletions tests/test_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
def _clean_env(monkeypatch):
"""Remove query-related env vars for test isolation."""
for key in (
'PLUTO_API_TOKEN',
'PLUTO_API_KEY',
'MLOP_API_TOKEN',
'PLUTO_URL_API',
'MLOP_URL_API',
Expand All @@ -42,7 +42,7 @@ def _make(status_code=200, json_data=None, text=''):
@pytest.fixture()
def client(monkeypatch):
"""A Client with a mocked httpx.Client."""
monkeypatch.setenv('PLUTO_API_TOKEN', 'test-token-123')
monkeypatch.setenv('PLUTO_API_KEY', 'test-token-123')
c = Client()
c._client = MagicMock(spec=httpx.Client)
return c
Expand All @@ -55,7 +55,7 @@ def client(monkeypatch):

class TestResolveApiToken:
def test_from_pluto_env(self, monkeypatch):
monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc')
monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc')
assert _resolve_api_token() == 'plt_abc'

def test_from_deprecated_mlop_env(self, monkeypatch):
Expand All @@ -69,7 +69,7 @@ def test_from_deprecated_mlop_env(self, monkeypatch):
assert any('MLOP_API_TOKEN' in str(x.message) for x in w)

def test_pluto_takes_precedence(self, monkeypatch):
monkeypatch.setenv('PLUTO_API_TOKEN', 'new_token')
monkeypatch.setenv('PLUTO_API_KEY', 'new_token')
monkeypatch.setenv('MLOP_API_TOKEN', 'old_token')
assert _resolve_api_token() == 'new_token'
Comment on lines 71 to 74

Choose a reason for hiding this comment

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

medium

If you implement my suggestion to add backward compatibility for PLUTO_API_TOKEN, please update the tests to cover the new logic. Specifically, this test should be expanded to ensure PLUTO_API_KEY has precedence over both PLUTO_API_TOKEN and MLOP_API_TOKEN. Additionally, new tests should be added to verify:

  1. A deprecation warning is issued when PLUTO_API_TOKEN is used.
  2. PLUTO_API_TOKEN has precedence over MLOP_API_TOKEN.


Expand Down Expand Up @@ -138,7 +138,7 @@ def test_requires_token(self):
Client()

def test_from_env(self, monkeypatch):
monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc')
monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc')
c = Client()
assert c._api_token == 'plt_abc'
c.close()
Expand All @@ -149,12 +149,12 @@ def test_explicit_token(self, monkeypatch):
c.close()

def test_context_manager(self, monkeypatch):
monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc')
monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc')
with Client() as c:
assert c._api_token == 'plt_abc'

def test_custom_host(self, monkeypatch):
monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc')
monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc')
c = Client(host='10.0.0.1')
assert c._url_api == 'http://10.0.0.1:3001'
c.close()
Expand Down Expand Up @@ -463,7 +463,7 @@ def test_connection_error_retries(self, client):

class TestModuleFunctions:
def test_list_runs_creates_default_client(self, monkeypatch, mock_response):
monkeypatch.setenv('PLUTO_API_TOKEN', 'test-token')
monkeypatch.setenv('PLUTO_API_KEY', 'test-token')

import pluto.query as pq

Expand Down
Loading