Skip to content
Open
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
13 changes: 11 additions & 2 deletions client/platform_client/eval_plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ class TaskNode(Cascade):

task_id: TaskRef
endpoints: list[Endpoint] | None = Field(default=None, min_length=1)
# This node's index in the plan's task list, stamped by `EvalPlan._number_the_nodes`.
position: int = Field(default=0, ge=0, exclude=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Derive each position from the task list

Rule earn-its-place violated:
TaskNode.position duplicates the index already encoded by EvalPlan.tasks; because these Pydantic models and their task lists remain mutable, reversing, inserting into, or appending to tasks after validation leaves the stored positions stale and can key coordinator records to the wrong nodes. Derive the position with enumerate() when filing nodes, or encapsulate all list mutation so numbering is recomputed.

AGENTS.md reference: AGENTS.md:L7-L8

Useful? React with 👍 / 👎.


@model_validator(mode='before')
@classmethod
Expand Down Expand Up @@ -223,8 +225,15 @@ def _states_a_count(self) -> Self:
return self

@model_validator(mode='after')
def _each_task_appears_once(self) -> Self:
_require_unique_names([task.task_id for task in self.tasks], 'the plan')
def _number_the_nodes(self) -> Self:
"""Stamp each node with its place in the list, which identifies it.

A plan may name one catalogue task twice, each node with its own scene, so `task_id` names
no single node and the position does. The stamp overwrites a value a caller sent: the index
is the only answer consistent with where the node sits.
"""
for position, task in enumerate(self.tasks):
task.position = position

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid mutating shared task nodes while numbering

Rule hidden-dependency violated:
_number_the_nodes assumes every list element is an independently owned TaskNode, but Pydantic retains already-constructed model instances: EvalPlan(tasks=[node, node], ...) therefore assigns through the same object twice and leaves both entries at position 1, while using a node in a later plan can silently rewrite its position in the earlier plan. Number distinct copies or keep the index outside the mutable node.

AGENTS.md reference: AGENTS.md:L7-L8

Useful? React with 👍 / 👎.

return self

@model_validator(mode='after')
Expand Down
28 changes: 25 additions & 3 deletions client/platform_client/tests/test_eval_plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,37 @@ def test_an_endpoint_says_whether_it_names_a_locator():
assert Endpoint(name='baseline', url='wss://x/ws').names_a_locator is True


def test_a_plan_names_each_task_and_each_endpoint_once():
with pytest.raises(ValidationError, match='more than once'):
a_plan(tasks=[SPOONS, SPOONS])
def test_a_plan_names_each_endpoint_once():
with pytest.raises(ValidationError, match='more than once'):
a_plan(endpoints=[BASELINE, BASELINE])
with pytest.raises(ValidationError, match='more than once'):
TaskNode.model_validate({'task_id': SPOONS, 'endpoints': [{'name': 'e'}, {'name': 'e'}]})


def test_a_plan_may_name_one_task_twice():
"""Two nodes of one task, each with its own scene, is a plan the platform runs. The position
keeps them apart; the id cannot."""
plan = a_plan(tasks=[SPOONS, SPOONS])

assert [task.task_id for task in plan.tasks] == [SPOONS, SPOONS]
assert [task.position for task in plan.tasks] == [0, 1]


def test_every_node_is_numbered_by_its_place_in_the_list():
plan = a_plan(tasks=[SPOONS, {'task_id': SPOONS, 'episodes_per_endpoint': 2}, SPOONS])

assert [task.position for task in plan.tasks] == [0, 1, 2]


def test_a_position_a_caller_states_is_the_list_order_regardless():
"""The index identifies the node, so a sent value cannot disagree with where it sits — and the
field never goes out on the wire for a caller to have sent one in the first place."""
plan = a_plan(tasks=[{'task_id': SPOONS, 'position': 7}, SPOONS])

assert [task.position for task in plan.tasks] == [0, 1]
assert 'position' not in plan.model_dump()['tasks'][0]


def test_a_task_endpoint_naming_no_locator_names_one_the_plan_defines():
with pytest.raises(ValidationError, match='name no endpoint the plan defines'):
a_plan(tasks=[{'task_id': SPOONS, 'endpoints': ['elsewhere']}])
Expand Down
2 changes: 1 addition & 1 deletion client/pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "positronic-platform-client"
version = "0.7.0"
version = "0.8.0"
description = "Typed wire contract and HTTP client for the Positronic evaluation platform API"
readme = "README.md"
requires-python = ">=3.11"
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ dependencies = [
# (.github/workflows/release.yaml). Pinned exactly, because that package guarantees no
# backwards compatibility: a floating requirement would let a fresh install of THIS release
# resolve a contract it was never built against.
"positronic-platform-client==0.7.0",
"positronic-platform-client==0.8.0",
"psutil", # `positronic-server` enumerates local interfaces to name a wildcard bind's addresses
"pyarrow",
"pydantic",
Expand Down
2 changes: 1 addition & 1 deletion uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading