Skip to content

Allow empty session name for APIs that take it #1057

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

Merged
merged 1 commit into from
May 2, 2025
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
27 changes: 27 additions & 0 deletions torchx/runner/test/api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
AppDef,
AppHandle,
AppState,
parse_app_handle,
Resource,
Role,
UnknownAppException,
Expand Down Expand Up @@ -132,6 +133,32 @@ def test_session_id(self, record_mock: MagicMock) -> None:
event = record_mock.call_args_list[i].args[0]
self.assertEqual(event.session, CURRENT_SESSION_ID)

def test_empty_session_id(self, _: MagicMock) -> None:
empty_session_name = ""
with Runner(
empty_session_name,
{"local": cast(SchedulerFactory, create_scheduler)},
) as runner:
app = AppDef(
"__unused_for_test__",
roles=[
Role(
name="echo",
image=str(self.tmpdir),
resource=resource.SMALL,
entrypoint="echo",
args=["__unused_for_test__"],
)
],
)

app_handle = runner.run(app, "local", self.cfg)

scheduler, session_name, app_id = parse_app_handle(app_handle)
self.assertEqual(scheduler, "local")
self.assertEqual(empty_session_name, session_name)
self.assertEqual(app_handle, f"local:///{app_id}")

def test_run(self, record_mock: MagicMock) -> None:
test_file = self.tmpdir / "test_file"

Expand Down
21 changes: 19 additions & 2 deletions torchx/specs/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1115,14 +1115,31 @@ def __init__(self, app_handle: "AppHandle") -> None:

def parse_app_handle(app_handle: AppHandle) -> ParsedAppHandle:
"""
parses the app handle into ```(scheduler_backend, session_name, and app_id)```
Parses the app handle into ```(scheduler_backend, session_name, and app_id)```.

Example:

.. doctest::

assert parse_app_handle("k8s://default/foo_bar") == ("k8s", "default", "foo_bar")
assert parse_app_handle("k8s:///foo_bar") == ("k8s", "", "foo_bar")

Args:
app_handle: a URI of the form ``{scheduler}://{session_name}/{app_id}``,
where the ``session_name`` is optional. In this case the app handle is
of the form ``{scheduler}:///{app_id}`` (notice the triple slashes).

Returns: A ``Tuple`` of three elements, ``(scheduler, session_name, app_id)``
parsed from the app_handle URI str. If the session name is not present then
an empty string is returned in its place in the tuple.

"""

# parse it manually b/c currently torchx does not
# define allowed characters nor length for session name and app_id
import re

pattern = r"(?P<scheduler_backend>.+)://(?P<session_name>.+)/(?P<app_id>.+)"
pattern = r"(?P<scheduler_backend>.+)://(?P<session_name>.*)/(?P<app_id>.+)"
match = re.match(pattern, app_handle)
if not match:
raise MalformedAppHandleException(app_handle)
Expand Down
9 changes: 9 additions & 0 deletions torchx/specs/test/api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,15 @@ def test_parse_malformed_app_handles(self) -> None:
with self.assertRaises(MalformedAppHandleException):
parse_app_handle(handle)

def test_parse_app_handle_empty_session_name(self) -> None:
# missing session name is not OK but an empty one is
app_handle = "local:///my_application_id"
handle = parse_app_handle(app_handle)

self.assertEqual(handle.app_id, "my_application_id")
self.assertEqual("local", handle.scheduler_backend)
self.assertEqual("", handle.session_name)

def test_parse(self) -> None:
(scheduler_backend, session_name, app_id) = parse_app_handle(
"local://my_session/my_app_id_1234"
Expand Down
Loading