Skip to content

Add ModalProvider(ContainerProvider)#821

Open
joyliu-q wants to merge 3 commits into
huggingface:mainfrom
joyliu-q:add-modal-provider
Open

Add ModalProvider(ContainerProvider)#821
joyliu-q wants to merge 3 commits into
huggingface:mainfrom
joyliu-q:add-modal-provider

Conversation

@joyliu-q

Copy link
Copy Markdown

Summary

Sample PR of adding Modal as a container provider to OpenEnv.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • New environment
  • Refactoring

Alignment Checklist

Before submitting, verify:

  • I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
  • I have checked .claude/docs/INVARIANTS.md and no invariants are violated
  • I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues
    • :flag: I ran this and actually a bunch of unrelated PRs failed lint formatting. We might want to fix this.

RFC Status

  • Not required (bug fix, docs, minor refactoring)
  • RFC exists: #___
  • RFC needed (will create before merge)

Test Plan

Claude Code Review

Usage

  """E2E check for ModalProvider against real Modal infra (echo_env)."""

  import functools

  import modal
  import requests

  # --- WORKAROUND: Modal's encrypted tunnel doesn't forward WS ping/pong frames,
  # so disable the websockets client keepalive for tunnelled connections. ---
  import openenv.core.env_client as _ec
  _ec.ws_connect = functools.partial(_ec.ws_connect, ping_interval=None)

  from openenv.core.containers.runtime.modal_provider import ModalProvider

  DOCKERFILE = "envs/echo_env/server/Dockerfile"   # path relative to repo root


  def run(use_v2: bool) -> None:
      print(f"\n=== ModalProvider e2e (use_sandbox_v2={use_v2}) ===")

      # ---- provider config ----
      provider = ModalProvider(
          app_name="openenv-e2e",     # Modal app (created if missing)
          use_sandbox_v2=use_v2,      # False -> Sandbox.create, True -> _experimental_create
          timeout=600,                # max sandbox lifetime (s)
          cpu=1.0,                    # optional
          memory=1024,                # optional; ignored when use_sandbox_v2=True
          # cmd="cd /app/env && uvicorn server.app:app --host 0.0.0.0 --port 8000",  # optional override
      )

      image = ModalProvider.image_from_dockerfile(DOCKERFILE)  # context defaults to envs/echo_env/
      print("image:", image)

      with modal.enable_output():                 # stream the build logs
          base_url = provider.start_container(image)
      print("base_url:", base_url)

      try:
          provider.wait_for_ready(base_url, timeout_s=180)
          print("health ready ✅")

          r = requests.get(f"{base_url}/health", timeout=10)
          assert r.status_code == 200, r.status_code
          print(f"GET /health -> {r.status_code} ✅")

          from echo_env.client import EchoEnv
          with EchoEnv(base_url=base_url).sync() as env:   # .sync() — client is async by default
              result = env.reset()
              print("reset ->", type(result).__name__, "✅")
          print(f"MCP round-trip OK (v2={use_v2}) ✅")
      finally:
          provider.stop_container()
          print("sandbox terminated")


  if __name__ == "__main__":
      run(use_v2=False)   # stable Sandbox API
      run(use_v2=True)    # Sandbox v2 (beta)
      print("\nALL E2E CHECKS PASSED ✅")

Run it (from the repo root, src + envs on the path, with modal available):

  PYTHONPATH=src:envs uv run --extra core --with modal python modal_e2e.py

@burtenshaw

burtenshaw commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR. I'm down with the direction but I think this should first align with the provider-neutral cloud sandbox shape from #793.

Core asks:

  • Make Modal work through stock EnvClient. The E2E currently relies on monkeypatching WebSocket ping_interval which imo we should avoid. lmk if we shoul make changes in core to facilitate sandbox services without the ws ping route.
  • Mirror ACA lifecycle semantics: reject double-starts, implement close()/context cleanup, and avoid masking startup errors during cleanup. See feat: add Azure Container Apps cloud sandbox provider #793
  • Treat sandbox logs and tunnel URLs as sensitive by default; redact or require explicit opt-in before surfacing logs.
  • Consider a small Modal SDK adapter/feature check, especially because Sandbox._experimental_create is private.
  • fix that PR example passes cpu/memory to ModalProvider(...), but the constructor rejects those; they currently must be passed to start_container()

Also, it would be nice to bring a simple example of inference with the modal sandbox, we can then build a training example from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants