diff --git a/scripts/prepare_hf_deployment.sh b/scripts/prepare_hf_deployment.sh index db151f458..618e0397a 100755 --- a/scripts/prepare_hf_deployment.sh +++ b/scripts/prepare_hf_deployment.sh @@ -788,15 +788,29 @@ deploy_env() { "$stage_dir" \ . || return 1 - # Ensure visibility is applied even when repo already existed. - local privacy_flag="--private" - if [ "$PRIVATE" = false ]; then - privacy_flag="--no-private" - fi + # Ensure visibility is applied even when repo already existed. `hf repos settings` + # rejects an update that changes nothing (and ignores `--no-private`), so only call + # it when visibility differs, with `--public` to make a Space public. + local space_info if [ ${#TOKEN_ARGS[@]} -gt 0 ]; then - hf repo settings --repo-type space "$privacy_flag" "${TOKEN_ARGS[@]}" "$space_repo" || return 1 + space_info=$(hf spaces info "$space_repo" --expand=private --format json "${TOKEN_ARGS[@]}") || return 1 else - hf repo settings --repo-type space "$privacy_flag" "$space_repo" || return 1 + space_info=$(hf spaces info "$space_repo" --expand=private --format json) || return 1 + fi + local current_private=false + if printf "%s" "$space_info" | grep -q '"private": *true'; then + current_private=true + fi + if [ "$current_private" != "$PRIVATE" ]; then + local privacy_flag="--private" + if [ "$PRIVATE" = false ]; then + privacy_flag="--public" + fi + if [ ${#TOKEN_ARGS[@]} -gt 0 ]; then + hf repos settings --repo-type space "$privacy_flag" "${TOKEN_ARGS[@]}" "$space_repo" || return 1 + else + hf repos settings --repo-type space "$privacy_flag" "$space_repo" || return 1 + fi fi DEPLOYED_SPACES+=("$space_repo") diff --git a/tests/scripts/test_prepare_hf_deployment.py b/tests/scripts/test_prepare_hf_deployment.py index 73316f79f..452bf048c 100644 --- a/tests/scripts/test_prepare_hf_deployment.py +++ b/tests/scripts/test_prepare_hf_deployment.py @@ -356,3 +356,78 @@ def test_prepare_hf_deployment_keeps_the_env_description_first(tmp_path: Path) - ) assert _description(meta).startswith("A chess reinforcement learning environment") assert "This Space is built from OpenEnv environment `chess_env`" in readme + + +@pytest.mark.parametrize( + ("current_private", "visibility_flag", "expected_settings_call"), + [ + ("false", "--public", None), + ("true", "--private", None), + ("true", "--public", "--public"), + ("false", "--private", "--private"), + ], +) +def test_prepare_hf_deployment_updates_visibility_only_when_it_differs( + tmp_path: Path, + current_private: str, + visibility_flag: str, + expected_settings_call: str | None, +) -> None: + """`hf repos settings` fails on an update that changes nothing and ignores + `--no-private`, so it only runs when visibility differs, with `--public`.""" + repo_root = Path(__file__).resolve().parents[2] + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls = tmp_path / "hf-calls.log" + fake_hf = bin_dir / "hf" + fake_hf.write_text( + "#!/usr/bin/env bash\n" + f'echo "$*" >> "{calls}"\n' + 'if [ "$1 $2" = "spaces info" ]; then\n' + f' echo \'{{"id": "openenv/echo_env", "private": {current_private}}}\'\n' + "fi\n" + 'if [ "$1 $2" = "repos settings" ]; then\n' + f' [ "{current_private}" = true ] && [[ " $* " == *" --private "* ]] && exit 1\n' + f' [ "{current_private}" = false ] && [[ " $* " == *" --public "* ]] && exit 1\n' + ' [[ " $* " == *" --no-private "* ]] && exit 1\n' + "fi\n" + "exit 0\n" + ) + fake_hf.chmod(0o755) + + result = subprocess.run( + [ + "bash", + str(repo_root / "scripts" / "prepare_hf_deployment.sh"), + "--env", + "echo_env", + "--repo-id", + "openenv/echo_env", + visibility_flag, + "--skip-collection", + "--staging-dir", + str(tmp_path / "hf-staging"), + ], + cwd=repo_root, + env={ + **os.environ, + "OPENENV_VERSION": "main", + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + }, + check=False, + capture_output=True, + text=True, + ) + + assert result.returncode == 0, result.stdout + result.stderr + assert "Uploaded https://huggingface.co/spaces/openenv/echo_env" in result.stdout + settings_calls = [ + line + for line in calls.read_text().splitlines() + if line.startswith("repos settings") + ] + if expected_settings_call is None: + assert settings_calls == [] + else: + assert len(settings_calls) == 1 + assert f" {expected_settings_call} " in f" {settings_calls[0]} "