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

Replace template string {resource_dir} in kernelspec #932

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
8 changes: 7 additions & 1 deletion jupyter_server/pytest_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -425,12 +425,13 @@ def client_fetch(*parts, headers=None, params=None, **kwargs):
"argv": ["cat", "{connection_file}"],
"display_name": "Test kernel",
}
sample_script = "cat"


@pytest.fixture
def jp_kernelspecs(jp_data_dir):
"""Configures some sample kernelspecs in the Jupyter data directory."""
spec_names = ["sample", "sample2", "bad"]
spec_names = ["sample", "sample2", "bad", "resource_dir"]
for name in spec_names:
sample_kernel_dir = jp_data_dir.joinpath("kernels", name)
sample_kernel_dir.mkdir(parents=True)
Expand All @@ -439,6 +440,11 @@ def jp_kernelspecs(jp_data_dir):
kernel_json = sample_kernel_json.copy()
if name == "bad":
kernel_json["argv"] = ["non_existent_path"]
elif name == "resource_dir":
sample_script_file = sample_kernel_dir.joinpath("sample.sh")
sample_script_file.write_text(sample_script)
sample_script_file.chmod(500)
kernel_json["argv"] = [os.path.join("{resource_dir}", "sample.sh")]
sample_kernel_file.write_text(json.dumps(kernel_json))
# Create resources text
sample_kernel_resources = sample_kernel_dir.joinpath("resource.txt")
Expand Down
15 changes: 15 additions & 0 deletions jupyter_server/services/kernelspecs/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@ def is_kernelspec_model(spec_dict):
)


def update_kernelspec_model(spec_dict):
"""Returns the original kernelspec unless template substitutions are available."""
model = spec_dict
if "resource_dir" in spec_dict:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure when this will be true since it's asking if the kernelspec model contains the key "resource_dir" yet, in the models, resource_dir is used to identify the other resources of the kernel (icon, etc.) and not retained as a key/value. As a result, this logic is probably better suited in kernelspec_model() directly.

I don't think this resource_dir-substitution logic applies to spec_dicts that satisfy the is_kernelspec_model() check because, for those, the water is under the bridge and the resource_dir is not available (nor is it local). The specs returned from a Gateway server are examples of those.

spec_str = json.dumps(spec_dict["spec"])
template_found = "{resource_dir}" in spec_str
resource_dir_exists = os.path.exists(spec_dict["resource_dir"])
if template_found and resource_dir_exists:
spec_str = spec_str.replace("{resource_dir}", spec_dict["resource_dir"])
Copy link
Member

Choose a reason for hiding this comment

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

This change will result in the substitution of {resource_dir} with the local value (see the previous comment about non-local specs returned from a Gateway) which typically occurs at the time of launch in jupyter_client.

(I don't see any issue with that but just wanted to make that comment in case others do.)

model["spec"] = json.loads(spec_str)
return model


class KernelSpecsAPIHandler(APIHandler):
auth_resource = AUTH_RESOURCE

Expand All @@ -73,6 +86,7 @@ async def get(self):
kernel_info["spec"],
kernel_info["resource_dir"],
)
d = update_kernelspec_model(d)
except Exception:
self.log.error("Failed to load kernel spec: '%s'", kernel_name, exc_info=True)
continue
Expand All @@ -95,6 +109,7 @@ async def get(self, kernel_name):
model = spec
else:
model = kernelspec_model(self, kernel_name, spec.to_dict(), spec.resource_dir)
model = update_kernelspec_model(model)
self.set_header("Content-Type", "application/json")
self.finish(json.dumps(model))

Expand Down
94 changes: 85 additions & 9 deletions tests/test_gateway.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,44 @@ def generate_kernelspec(name):
return kernelspec_stanza


# We'll mock up two kernelspecs - kspec_foo and kspec_bar
kernelspecs: dict = {
"default": "kspec_foo",
"kernelspecs": {
"kspec_foo": generate_kernelspec("kspec_foo"),
"kspec_bar": generate_kernelspec("kspec_bar"),
},
}
def generate_kernelspec_with_resourcedir(name, spec_dir):
# create a file that simply calls the standard argv stanza
contents = "python -m ipykernel_launcher -f {connection_file}"
with open(spec_dir.joinpath("launcher.sh"), "w") as resource_dir_script:
resource_dir_script.write(contents)
argv_stanza = [os.path.join("{resource_dir}", "launcher.sh")]
kernelspec_stanza = generate_kernelspec(name)
kernelspec_stanza["spec"]["spec"]["argv"] = argv_stanza
kernelspec_stanza["resource_dir"] = str(spec_dir)
return kernelspec_stanza


kernelspecs: dict = {}


@pytest.fixture(autouse=True)
def init_kernelspecs(jp_data_dir):
global kernelspecs

spec_dir = jp_data_dir.joinpath("kernels", "kspec_resourcedir")
if not spec_dir.exists():
spec_dir.mkdir(parents=True)

missing_rdir = generate_kernelspec_with_resourcedir("kspec_resourcedir_missing", spec_dir)
del missing_rdir["resource_dir"]

# mock kernelspecs
kernelspecs = {
"default": "kspec_foo",
"kernelspecs": {
"kspec_foo": generate_kernelspec("kspec_foo"),
"kspec_bar": generate_kernelspec("kspec_bar"),
"kspec_resourcedir": generate_kernelspec_with_resourcedir(
"kspec_resourcedir", spec_dir
),
"kspec_resourcedir_missing": missing_rdir,
},
}


# maintain a dictionary of expected running kernels. Key = kernel_id, Value = model.
Expand Down Expand Up @@ -273,10 +303,30 @@ async def test_gateway_get_kernelspecs(init_gateway, jp_fetch):
assert r.code == 200
content = json.loads(r.body.decode("utf-8"))
kspecs = content.get("kernelspecs")
assert len(kspecs) == 2
assert len(kspecs) == 4
assert kspecs.get("kspec_bar").get("name") == "kspec_bar"


async def test_gateway_get_kernelspec_filled_resource_dir(init_gateway, jp_fetch):
# Validate that kernelspec with resource_dir filled came from gateway.
with mocked_gateway:
r = await jp_fetch("api", "kernelspecs", "kspec_resourcedir", method="GET")
assert r.code == 200
kspec_resourcedir = json.loads(r.body.decode("utf-8"))
assert kspec_resourcedir.get("name") == "kspec_resourcedir"
assert "{resource_dir}" not in json.dumps(kspec_resourcedir.get("spec"))


async def test_gateway_get_kernelspec_unfilled_resource_dir(init_gateway, jp_fetch):
# Validate that kernelspec with resource_dir unfilled came from gateway.
with mocked_gateway:
r = await jp_fetch("api", "kernelspecs", "kspec_resourcedir_missing", method="GET")
assert r.code == 200
kspec_resourcedir_missing = json.loads(r.body.decode("utf-8"))
assert kspec_resourcedir_missing.get("name") == "kspec_resourcedir_missing"
assert "{resource_dir}" in json.dumps(kspec_resourcedir_missing.get("spec"))


async def test_gateway_get_named_kernelspec(init_gateway, jp_fetch):
# Validate that a specific kernelspec can be retrieved from gateway (and an invalid spec can't)
with mocked_gateway:
Expand Down Expand Up @@ -342,6 +392,32 @@ async def test_gateway_kernel_lifecycle(init_gateway, jp_fetch):
assert await is_kernel_running(jp_fetch, kernel_id) is False


async def test_gateway_kernel_lifecycle_resource_dir(init_gateway, jp_fetch):
# Validate kernel lifecycle functions; create, interrupt, restart and delete for resource_dir template use.

# create
kernel_id = await create_kernel(jp_fetch, "kspec_resourcedir")

# ensure kernel still considered running
assert await is_kernel_running(jp_fetch, kernel_id) is True

# interrupt
await interrupt_kernel(jp_fetch, kernel_id)

# ensure kernel still considered running
assert await is_kernel_running(jp_fetch, kernel_id) is True

# restart
await restart_kernel(jp_fetch, kernel_id)

# ensure kernel still considered running
assert await is_kernel_running(jp_fetch, kernel_id) is True

# delete
await delete_kernel(jp_fetch, kernel_id)
assert await is_kernel_running(jp_fetch, kernel_id) is False


@pytest.mark.parametrize("missing_kernel", [True, False])
async def test_gateway_shutdown(init_gateway, jp_serverapp, jp_fetch, missing_kernel):
# Validate server shutdown when multiple gateway kernels are present or
Expand Down