Skip to content
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

Getting the CI to pass consistently on Windows (hopefully) #4624

Merged
merged 19 commits into from Jun 22, 2023
71 changes: 1 addition & 70 deletions .github/workflows/backend.yml
Expand Up @@ -39,81 +39,12 @@ jobs:
- '.github/**'
scripts:
- 'scripts/**'
client-test:
needs: [changes]
if: needs.changes.outputs.python-client == 'true' || needs.changes.outputs.workflows == 'true'
strategy:
matrix:
os: ["ubuntu-latest", "windows-latest"]
test-type: ["not flaky", "flaky"]
python-version: ["3.8"]
runs-on: ${{ matrix.os }}
continue-on-error: ${{ matrix.test-type == 'flaky' }}
defaults:
run:
working-directory: client/python
steps:
- uses: actions/checkout@v3
- name: Install Python
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: pip
cache-dependency-path: |
client/python/requirements.txt
requirements.txt
test/requirements.txt
- name: Create env
run: |
python -m pip install --upgrade virtualenv
python -m virtualenv venv
- uses: actions/cache@master
id: cache
with:
path: |
client/python/venv/*
key: python-client-${{ runner.os }}-pip-${{ hashFiles('client/python/requirements.txt') }}-${{ hashFiles('client/python/test/requirements.txt') }}
- name: Install Test Requirements (Linux)
if: steps.cache.outputs.cache-hit != 'true' && runner.os == 'Linux'
run: |
. venv/bin/activate
python -m pip install -r test/requirements.txt
- name: Install Client Library (Linux)
if: runner.os == 'Linux'
run: |
. venv/bin/activate
python -m pip install -e .
- name: Lint (Linux)
if: runner.os == 'Linux'
run: |
. venv/bin/activate
bash scripts/lint.sh
- name: Tests (Linux)
if: runner.os == 'Linux'
run: |
. venv/bin/activate
python -m pytest -m "${{ matrix.test-type }}"
- name: Install Test Requirements (Windows)
if: steps.cache.outputs.cache-hit != 'true' && runner.os == 'Windows'
run: |
venv\Scripts\activate
pip install -r test/requirements.txt
- name: Install Client Library (Windows)
if: runner.os == 'Windows'
run: |
venv\Scripts\activate
pip install -e .
- name: Tests (Windows)
if: runner.os == 'Windows'
run: |
venv\Scripts\activate
python -m pytest -m "${{ matrix.test-type }}"
test:
needs: [changes]
if: needs.changes.outputs.gradio == 'true' || needs.changes.outputs.workflows == 'true' || needs.changes.outputs.scripts == 'true' || needs.changes.outputs.test == 'true'
strategy:
matrix:
os: ["ubuntu-latest", "windows-latest"]
os: ["windows-latest"]
test-type: ["not flaky", "flaky"]
python-version: ["3.8"]
runs-on: ${{ matrix.os }}
Expand Down
2 changes: 0 additions & 2 deletions gradio/blocks.py
Expand Up @@ -665,9 +665,7 @@ def __init__(
title: The tab title to display when this is opened in a browser window.
css: custom css or path to custom css file to apply to entire Blocks
"""
# Cleanup shared parameters with Interface #TODO: is this part still necessary after Interface with Blocks?
self.limiter = None
self.save_to = None
if theme is None:
theme = DefaultTheme()
elif isinstance(theme, str):
Expand Down
4 changes: 4 additions & 0 deletions gradio/exceptions.py
Expand Up @@ -19,6 +19,10 @@ class InvalidApiNameError(ValueError):
pass


class ServerFailedToStartError(Exception):
pass


InvalidApiName = InvalidApiNameError # backwards compatibility


Expand Down
1 change: 0 additions & 1 deletion gradio/interface.py
Expand Up @@ -363,7 +363,6 @@ def __init__(
self.max_batch_size = max_batch_size
self.allow_duplication = allow_duplication

self.save_to = None # Used for selenium tests
self.share = None
self.share_url = None
self.local_url = None
Expand Down
94 changes: 54 additions & 40 deletions gradio/networking.py
Expand Up @@ -14,6 +14,7 @@
import requests
import uvicorn

from gradio.exceptions import ServerFailedToStartError
from gradio.routes import App
from gradio.tunneling import Tunnel

Expand All @@ -35,8 +36,13 @@ def install_signal_handlers(self):
def run_in_thread(self):
self.thread = threading.Thread(target=self.run, daemon=True)
self.thread.start()
start = time.time()
while not self.started:
time.sleep(1e-3)
if time.time() - start > 5:
Copy link
Member Author

Choose a reason for hiding this comment

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

It takes <10 ms on my computer to start the server. So 5 seconds is a very generous allotment for slow computers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Might be good to add "to start within 5 seconds." for more context. Will push this up since you're away

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually this message is not displayed to the user so won't make the change

raise ServerFailedToStartError(
"Server failed to start. Please check that the port is available."
)

def close(self):
self.should_exit = True
Expand Down Expand Up @@ -108,36 +114,12 @@ def start_server(
app: the FastAPI app object
server: the server object that is a subclass of uvicorn.Server (used to close the server)
"""
server_name = server_name or LOCALHOST_NAME
# if port is not specified, search for first available port
if server_port is None:
port = get_first_available_port(
Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't removed this method get_first_available_port from the code since we use it in reload mode. I do think we need to overhaul reload mode at some point, and then we could remove this method too

INITIAL_PORT_VALUE, INITIAL_PORT_VALUE + TRY_NUM_PORTS
)
else:
try:
s = socket.socket()
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
s.bind((LOCALHOST_NAME, server_port))
s.close()
except OSError as err:
raise OSError(
f"Port {server_port} is in use. If a gradio.Blocks is running on the port, "
f"you can close() it or gradio.close_all()."
) from err
port = server_port
if ssl_keyfile is not None and ssl_certfile is None:
raise ValueError("ssl_certfile must be provided if ssl_keyfile is provided.")

server_name = server_name or LOCALHOST_NAME
url_host_name = "localhost" if server_name == "0.0.0.0" else server_name

if ssl_keyfile is not None:
if ssl_certfile is None:
raise ValueError(
"ssl_certfile must be provided if ssl_keyfile is provided."
)
path_to_local_server = f"https://{url_host_name}:{port}/"
else:
path_to_local_server = f"http://{url_host_name}:{port}/"

# Strip IPv6 brackets from the address if they exist.
# This is needed as http://[::1]:port/ is a valid browser address,
# but not a valid IPv6 address, so asyncio will throw an exception.
Expand All @@ -148,20 +130,52 @@ def start_server(

app = App.create_app(blocks, app_kwargs=app_kwargs)

if blocks.save_to is not None: # Used for selenium tests
blocks.save_to["port"] = port
config = uvicorn.Config(
app=app,
port=port,
host=host,
log_level="warning",
ssl_keyfile=ssl_keyfile,
ssl_certfile=ssl_certfile,
ssl_keyfile_password=ssl_keyfile_password,
ws_max_size=1024 * 1024 * 1024, # Setting max websocket size to be 1 GB
# if port is not specified, search for an open port in the range of 7860 to 7959
server_ports = (
[server_port]
if server_port is not None
else range(INITIAL_PORT_VALUE, INITIAL_PORT_VALUE + TRY_NUM_PORTS)
)
server = Server(config=config)
server.run_in_thread()

for port in server_ports:
try:
# The fastest way to check if a port is available is to try to bind to it with socket.
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the copious comments, but I wanted to make sure the new logic is clear to someone who is reading the code for the first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate it!

# If the port is not available, socket will throw an OSError.
s = socket.socket()
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
# Really, we should be checking if (server_name, server_port) is available, but
# socket.bind() doesn't seem to throw an OSError with ipv6 addresses, based on my testing.
# Instead, we just check if the port is available on localhost.
s.bind((LOCALHOST_NAME, port))
s.close()

# To avoid race conditions, so we also check if the port by trying to start the uvicorn server.
# If the port is not available, this will throw a ServerFailedToStartError.
config = uvicorn.Config(
app=app,
port=port,
host=host,
log_level="warning",
ssl_keyfile=ssl_keyfile,
ssl_certfile=ssl_certfile,
ssl_keyfile_password=ssl_keyfile_password,
ws_max_size=1024 * 1024 * 1024, # Setting max websocket size to be 1 GB
)
server = Server(config=config)
server.run_in_thread()
break
except (OSError, ServerFailedToStartError):
pass
else:
raise OSError(
f"Cannot find empty port in range: {min(server_ports)}-{max(server_ports)}. You can specify a different port by setting the GRADIO_SERVER_PORT environment variable or passing the `server_port` parameter to `launch()`."
)

if ssl_keyfile is not None:
path_to_local_server = f"https://{url_host_name}:{port}/"
else:
path_to_local_server = f"http://{url_host_name}:{port}/"

return server_name, port, path_to_local_server, app, server


Expand Down