fix: validate script_path exists + tell agent to call generate_job_file
Two problems with the prior prepare_submit_job flow:
1. The agent could pass a script_path that only existed in its own
context (LLM-generated code not yet on disk) or a path on the host
filesystem that's invisible inside the container. The new check
surfaces this as a 400 with a clear remediation hint instead of
letting spark-submit fail later with an opaque FileNotFoundError -> 500.
2. The container-isolation issue: any path the agent gives is interpreted
inside the container. The two ways a file can legitimately exist there
are (a) generate_job_file(code=...) just wrote it to
SPARK_EXECUTOR_JOBS_DIR (the default ./data/jobs/ is the only
gitignored dir that survives restarts), or (b) a host dir was
mounted via -v. The error message spells both out so an agent can
self-correct.
Implementation:
- submit.py: new _check_script_path() that raises ValueError (-> 400)
when the path is missing, empty, or a directory. Called in both
prepare_submit_job and confirm_submit_job (defense in depth).
- prepare_submit_job checks the connection FIRST (KeyError -> 404)
before the script (ValueError -> 400), so an agent with both problems
sees the more fundamental 'unknown connection' error first.
- server.py / requests.py: route description and Pydantic field
description spell out the generate_job_file pattern so an LLM
reading the tool schema learns the right next step.
8 new tests; 8 existing tests adjusted to create real files (they used
synthetic /tmp/*.py paths that don't exist).
This commit is contained in:
@@ -26,6 +26,14 @@ def _fresh(tmp_path: Path, monkeypatch):
|
||||
))
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def real_script(tmp_path: Path) -> Path:
|
||||
"""A real .py file on disk that satisfies prepare_submit_job's path check."""
|
||||
p = tmp_path / "demo.py"
|
||||
p.write_text("print('hi from test')\n")
|
||||
return p
|
||||
|
||||
|
||||
def _last_pending_id() -> str:
|
||||
pending = submit.pending_store.list_all()
|
||||
assert pending, "no pending submission was created"
|
||||
@@ -34,16 +42,16 @@ def _last_pending_id() -> str:
|
||||
|
||||
# --- prepare_submit_job ---
|
||||
|
||||
def test_prepare_does_not_invoke_spark_submit(monkeypatch):
|
||||
def test_prepare_does_not_invoke_spark_submit(monkeypatch, real_script):
|
||||
called = []
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda cmd: called.append(cmd))
|
||||
out = submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
out = submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
assert called == [] # never called
|
||||
assert out["status"] == "PENDING"
|
||||
assert out["pending_id"].startswith("p_")
|
||||
|
||||
|
||||
def test_prepare_persists_pending_with_snapshot(monkeypatch):
|
||||
def test_prepare_persists_pending_with_snapshot(monkeypatch, real_script):
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda cmd: None)
|
||||
connection_store.store.save(
|
||||
Connection(
|
||||
@@ -54,7 +62,7 @@ def test_prepare_persists_pending_with_snapshot(monkeypatch):
|
||||
spark_conf={"spark.sql.shuffle.partitions": "200"},
|
||||
)
|
||||
)
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py", queue="research")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script), queue="research")
|
||||
p = submit.pending_store.get(_last_pending_id())
|
||||
assert p is not None
|
||||
assert p.connection == "prod"
|
||||
@@ -63,18 +71,57 @@ def test_prepare_persists_pending_with_snapshot(monkeypatch):
|
||||
assert p.yarn_rm_url == "http://rm:8088"
|
||||
assert p.spark_conf == {"spark.sql.shuffle.partitions": "200"}
|
||||
assert p.queue == "research"
|
||||
assert p.script_path == "/tmp/j.py"
|
||||
assert p.script_path == str(real_script)
|
||||
|
||||
|
||||
def test_prepare_raises_for_unknown_connection():
|
||||
def test_prepare_raises_for_unknown_connection(real_script):
|
||||
with pytest.raises(KeyError, match="missing"):
|
||||
submit.prepare_submit_job(connection="missing", script_path="/tmp/j.py")
|
||||
submit.prepare_submit_job(connection="missing", script_path=str(real_script))
|
||||
|
||||
|
||||
def test_prepare_snapshots_connection_at_prepare_time(monkeypatch):
|
||||
# --- script_path validation ---
|
||||
|
||||
def test_prepare_rejects_nonexistent_script_path(tmp_path):
|
||||
missing = str(tmp_path / "does_not_exist.py")
|
||||
with pytest.raises(ValueError, match="does not exist or is not a file"):
|
||||
submit.prepare_submit_job(connection="prod", script_path=missing)
|
||||
|
||||
|
||||
def test_prepare_rejects_directory_as_script_path(tmp_path):
|
||||
"""A directory is not a file, even if it exists."""
|
||||
with pytest.raises(ValueError, match="does not exist or is not a file"):
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(tmp_path))
|
||||
|
||||
|
||||
def test_prepare_rejects_empty_script_path():
|
||||
with pytest.raises(ValueError, match="does not exist or is not a file"):
|
||||
submit.prepare_submit_job(connection="prod", script_path="")
|
||||
|
||||
|
||||
def test_prepare_error_message_mentions_generate_job_file(tmp_path):
|
||||
"""The agent must be told to call generate_job_file first."""
|
||||
with pytest.raises(ValueError, match="generate_job_file"):
|
||||
submit.prepare_submit_job(
|
||||
connection="prod", script_path=str(tmp_path / "x.py")
|
||||
)
|
||||
|
||||
|
||||
def test_confirm_rejects_if_script_was_deleted_after_prepare(tmp_path, monkeypatch):
|
||||
"""Defense in depth: file present at prepare, gone by confirm -> 400."""
|
||||
script = tmp_path / "demo.py"
|
||||
script.write_text("print('hi')\n")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(script))
|
||||
pid = _last_pending_id()
|
||||
# Simulate the file being deleted (e.g. cleanup job) between prepare and confirm
|
||||
script.unlink()
|
||||
with pytest.raises(ValueError, match="does not exist or is not a file"):
|
||||
submit.confirm_submit_job(pending_id=pid)
|
||||
|
||||
|
||||
def test_prepare_snapshots_connection_at_prepare_time(monkeypatch, real_script):
|
||||
"""Editing the connection between prepare and confirm must NOT silently retarget."""
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda cmd: None)
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
p = submit.pending_store.get(_last_pending_id())
|
||||
# Now mutate the connection
|
||||
connection_store.store.save(
|
||||
@@ -88,8 +135,8 @@ def test_prepare_snapshots_connection_at_prepare_time(monkeypatch):
|
||||
|
||||
# --- confirm_submit_job ---
|
||||
|
||||
def test_confirm_invokes_spark_submit_and_marks_submitted(monkeypatch):
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
def test_confirm_invokes_spark_submit_and_marks_submitted(monkeypatch, real_script):
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
pid = _last_pending_id()
|
||||
fake_proc = type("P", (), {
|
||||
"returncode": 0,
|
||||
@@ -100,7 +147,7 @@ def test_confirm_invokes_spark_submit_and_marks_submitted(monkeypatch):
|
||||
cmd = m.call_args.args[0]
|
||||
assert "yarn" in cmd
|
||||
assert "cluster" in cmd
|
||||
assert cmd[-1] == "/tmp/j.py"
|
||||
assert cmd[-1] == str(real_script)
|
||||
assert result.application_id == "application_17400000001"
|
||||
# pending updated
|
||||
p = submit.pending_store.get(pid)
|
||||
@@ -116,8 +163,8 @@ def test_confirm_raises_for_unknown_pending_id():
|
||||
submit.confirm_submit_job(pending_id="missing")
|
||||
|
||||
|
||||
def test_confirm_refuses_non_pending_status(monkeypatch):
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
def test_confirm_refuses_non_pending_status(monkeypatch, real_script):
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
pid = _last_pending_id()
|
||||
# Mark it CANCELLED first
|
||||
p = submit.pending_store.get(pid)
|
||||
@@ -127,9 +174,9 @@ def test_confirm_refuses_non_pending_status(monkeypatch):
|
||||
submit.confirm_submit_job(pending_id=pid)
|
||||
|
||||
|
||||
def test_confirm_marks_failed_on_spark_submit_error(monkeypatch):
|
||||
def test_confirm_marks_failed_on_spark_submit_error(monkeypatch, real_script):
|
||||
from spark_executor.core.spark_submit import SparkSubmitError
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
pid = _last_pending_id()
|
||||
def _raise(_cmd):
|
||||
raise SparkSubmitError("boom")
|
||||
@@ -147,20 +194,23 @@ def test_list_pending_jobs_empty():
|
||||
assert submit.list_pending_jobs() == []
|
||||
|
||||
|
||||
def test_list_pending_jobs_returns_all(monkeypatch):
|
||||
def test_list_pending_jobs_returns_all(monkeypatch, tmp_path):
|
||||
a = tmp_path / "a.py"
|
||||
b = tmp_path / "b.py"
|
||||
a.write_text("a\n"); b.write_text("b\n")
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda cmd: None)
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/a.py")
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/b.py")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(a))
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(b))
|
||||
out = submit.list_pending_jobs()
|
||||
assert {p["script_path"] for p in out} == {"/tmp/a.py", "/tmp/b.py"}
|
||||
assert {p["script_path"] for p in out} == {str(a), str(b)}
|
||||
assert all(p["status"] == "PENDING" for p in out)
|
||||
|
||||
|
||||
# --- get_pending_job ---
|
||||
|
||||
def test_get_pending_job_returns_dump(monkeypatch):
|
||||
def test_get_pending_job_returns_dump(monkeypatch, real_script):
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda cmd: None)
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
pid = _last_pending_id()
|
||||
out = submit.get_pending_job(pid)
|
||||
assert out["pending_id"] == pid
|
||||
@@ -175,9 +225,9 @@ def test_get_pending_job_unknown_raises():
|
||||
|
||||
# --- cancel_pending_job ---
|
||||
|
||||
def test_cancel_pending_job_marks_cancelled(monkeypatch):
|
||||
def test_cancel_pending_job_marks_cancelled(monkeypatch, real_script):
|
||||
monkeypatch.setattr(submit, "run_spark_submit", lambda cmd: None)
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
pid = _last_pending_id()
|
||||
out = submit.cancel_pending_job(pid)
|
||||
assert out == {"pending_id": pid, "status": "CANCELLED"}
|
||||
@@ -189,8 +239,8 @@ def test_cancel_pending_job_unknown_raises():
|
||||
submit.cancel_pending_job("missing")
|
||||
|
||||
|
||||
def test_cancel_pending_job_refuses_submitted(monkeypatch):
|
||||
submit.prepare_submit_job(connection="prod", script_path="/tmp/j.py")
|
||||
def test_cancel_pending_job_refuses_submitted(monkeypatch, real_script):
|
||||
submit.prepare_submit_job(connection="prod", script_path=str(real_script))
|
||||
pid = _last_pending_id()
|
||||
p = submit.pending_store.get(pid)
|
||||
p.status = "SUBMITTED"
|
||||
|
||||
Reference in New Issue
Block a user