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
28 changes: 13 additions & 15 deletions synapseclient/models/evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ class Evaluation(EvaluationSynchronousProtocol):
etag: Synapse employs an Optimistic Concurrency Control (OCC) scheme to handle concurrent updates.
The eTag changes every time an Evaluation is updated; it is used to detect when a client's copy
of an Evaluation is out-of-date.
name: The name of this Evaluation.
description: A text description of this Evaluation.
name: **Required to store.** The name of this Evaluation.
Comment thread
jaymedina marked this conversation as resolved.
description: **Required to store.** A text description of this Evaluation.
owner_id: The ID of the Synapse user who created this Evaluation.
created_on: The date on which Evaluation was created.
content_source: The Synapse ID of the Entity to which this Evaluation belongs,
content_source: **Required to store.** The Synapse ID of the Entity to which this Evaluation belongs,
e.g. a reference to a Synapse project.
submission_instructions_message: Message to display to users detailing acceptable formatting for Submissions to this Evaluation.
submission_receipt_message: Message to display to users upon successful submission to this Evaluation.
Expand All @@ -56,8 +56,6 @@ class Evaluation(EvaluationSynchronousProtocol):
name="My Challenge Evaluation",
description="Evaluation for my data challenge",
content_source="syn123456",
submission_instructions_message="Submit CSV files only",
submission_receipt_message="Thank you for your submission!",
)
created = evaluation.store()
```
Expand Down Expand Up @@ -123,10 +121,10 @@ class Evaluation(EvaluationSynchronousProtocol):
of an Evaluation is out-of-date."""

name: Optional[str] = None
"""The name of this Evaluation."""
"""**Required to store.** The name of this Evaluation."""
Comment on lines 123 to +124

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For discussion, should there be an Optional[str] here if it is "required"? Is Required to store a new semantic within the client?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the idea is that "name" is required when calling evaluation.store(). But I think we don't have the convention of doing something like **Required to store.** in the code base. My understanding is that we will just raise an error like here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with Lingling here on both counts.

@thomasyu888 thomasyu888 Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should type hinting be...

name: None
    """The name of this Evaluation."""

instead of

name: Optional[str] = None


description: Optional[str] = None
"""A text description of this Evaluation."""
"""**Required to store.** A text description of this Evaluation."""

owner_id: Optional[str] = None
"""The ID of the Synapse user who created this Evaluation."""
Expand All @@ -135,7 +133,7 @@ class Evaluation(EvaluationSynchronousProtocol):
"""The date on which Evaluation was created."""

content_source: Optional[str] = None
"""The Synapse ID of the Entity to which this Evaluation belongs,
"""**Required to store.** The Synapse ID of the Entity to which this Evaluation belongs,
e.g. a reference to a Synapse project."""

submission_instructions_message: Optional[str] = None
Expand Down Expand Up @@ -255,8 +253,6 @@ def to_synapse_request(self, request_type: RequestType):
"name",
"description",
"content_source",
"submission_instructions_message",
"submission_receipt_message",
]

# For "update" requests, add id and etag
Expand All @@ -274,9 +270,13 @@ def to_synapse_request(self, request_type: RequestType):
"name": self.name,
"description": self.description,
"contentSource": self.content_source,
"submissionInstructionsMessage": self.submission_instructions_message,
"submissionReceiptMessage": self.submission_receipt_message,
}
if self.submission_instructions_message is not None:
request_body["submissionInstructionsMessage"] = (
self.submission_instructions_message
)
if self.submission_receipt_message is not None:
request_body["submissionReceiptMessage"] = self.submission_receipt_message

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't delete_none_keys(request_body) be used here?

# For UPDATE request types, add id and etag
if request_type == RequestType.UPDATE:
Expand Down Expand Up @@ -307,7 +307,7 @@ async def store_async(

Example: Creating a new evaluation
 
Create a new evaluation on Synapse by storing an evaluation object with the required fields. If there are any fields missing, an error will be raised.
Create a new evaluation on Synapse by storing an evaluation object with the required fields.
```python
from synapseclient.models import Evaluation
from synapseclient import Synapse
Expand All @@ -321,8 +321,6 @@ async def create_evaluation():
name="My Challenge Evaluation",
description="Evaluation for my data challenge",
content_source="syn123456",
submission_instructions_message="Submit CSV files only",
submission_receipt_message="Thank you for your submission!"
).store_async()

return evaluation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,23 @@ def init(self, syn: Synapse, schedule_for_cleanup: Callable[..., None]) -> None:
self.syn = syn
self.schedule_for_cleanup = schedule_for_cleanup

async def test_create_evaluation(self):
# GIVEN a project to work with
@pytest.fixture(scope="class")
async def test_project(
self, syn: Synapse, schedule_for_cleanup: Callable[..., None]
) -> Project:
"""Create a test project for evaluation creation tests."""
project = await Project(name=f"test_project_{uuid.uuid4()}").store_async(
synapse_client=self.syn
synapse_client=syn
)
self.schedule_for_cleanup(project.id)
schedule_for_cleanup(project.id)
return project

async def test_create_evaluation(self, test_project: Project):
# WHEN I create an evaluation using the dataclass method
evaluation = Evaluation(
name=f"test_evaluation_{uuid.uuid4()}",
description="A test evaluation for testing purposes",
content_source=project.id,
content_source=test_project.id,
submission_instructions_message="Please submit your results in CSV format",
submission_receipt_message="Thank you for your submission!",
)
Expand All @@ -42,10 +47,28 @@ async def test_create_evaluation(self):
assert (
created_evaluation.description == "A test evaluation for testing purposes"
)
assert created_evaluation.content_source == project.id
assert created_evaluation.content_source == test_project.id
assert created_evaluation.owner_id is not None # Check that owner_id is set

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the entire TestEvaluationValidation class should also probably be moved to unit test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, test_create_evaluation_missing_required_fields only covers when the description is missing. But missing name and missing content_source aren't tested.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think def _build_evaluation_with_all_fields(self) -> Evaluation and probably some other tests related to test_to_synapse_request_create should also be updated.

assert created_evaluation.created_on is not None # Check that created_on is set

async def test_create_evaluation_without_optional_message_fields(
self, test_project: Project
):
# WHEN I create an evaluation without submission_instructions_message or submission_receipt_message
evaluation = Evaluation(
name=f"test_evaluation_{uuid.uuid4()}",
description="A test evaluation without optional message fields",
content_source=test_project.id,
)
created_evaluation = await evaluation.store_async(synapse_client=self.syn)
self.schedule_for_cleanup(created_evaluation.id)

# THEN the evaluation should be created successfully
assert created_evaluation.id is not None
assert created_evaluation.etag is not None
assert created_evaluation.name == evaluation.name
assert created_evaluation.content_source == test_project.id


class TestGetEvaluation:
@pytest.fixture(autouse=True, scope="function")
Expand Down Expand Up @@ -294,6 +317,31 @@ async def test_update_multiple_fields(self, test_evaluation: Evaluation):
assert updated_evaluation.etag is not None
assert updated_evaluation.etag != old_etag

async def test_update_evaluation_without_optional_message_fields(
self, test_project: Project
):
# GIVEN an evaluation created without submission message fields
evaluation = Evaluation(
name=f"test_evaluation_{uuid.uuid4()}",
description="A test evaluation without optional messages",
content_source=test_project.id,
)
created_evaluation = await evaluation.store_async(synapse_client=self.syn)
self.schedule_for_cleanup(created_evaluation.id)

# WHEN I update a field on that evaluation (still without message fields)
new_description = f"Updated description {uuid.uuid4()}"
created_evaluation.description = new_description
updated_evaluation = await created_evaluation.store_async(
synapse_client=self.syn
)

# THEN the update should succeed
assert updated_evaluation.description == new_description
assert updated_evaluation.id == created_evaluation.id
assert updated_evaluation.submission_instructions_message is None
assert updated_evaluation.submission_receipt_message is None
Comment thread
jaymedina marked this conversation as resolved.

async def test_store_unchanged_evaluation(
self, test_evaluation: Evaluation, monkeypatch
):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,40 @@ async def test_store_submission_successfully_async(
assert created_submission.created_on is not None
assert created_submission.version_number is not None

async def test_store_submission_to_evaluation_without_message_fields_async(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: the submission tests shouldn't care whether submission_instructions_message or submission_receipt_message are set. What about just removing the message fields from test_evaluation, delete test_store_submission_to_evaluation_without_message_fields_async

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: This comment has been generated with AI assistance and reviewed by me ✍️

The intent of test_store_submission_to_evaluation_without_message_fields_async is to ensure that the submission service doesn't require submission_instructions_message or submission_receipt_message to be set on the evaluation — since those attributes surface to submitters, it's worth explicitly verifying that submissions can be created against an evaluation that omits them. Happy to keep the test and clarify the comment if that makes the intent clearer.

self,
test_project: Project,
test_file: File,
):
# GIVEN an evaluation created without submission_instructions_message or submission_receipt_message
evaluation = Evaluation(
name=f"test_evaluation_{uuid.uuid4()}",
description="Evaluation without optional message fields",
content_source=test_project.id,
)
created_evaluation = await evaluation.store_async(synapse_client=self.syn)
self.schedule_for_cleanup(created_evaluation.id)

# WHEN I submit to that evaluation
submission = Submission(
entity_id=test_file.id,
evaluation_id=created_evaluation.id,
name=f"Test Submission {uuid.uuid4()}",
)
created_submission = await submission.store_async(synapse_client=self.syn)
self.schedule_for_cleanup(created_submission.id)

# THEN the submission should be created successfully
assert created_submission.id is not None
assert created_submission.entity_id == test_file.id
assert created_submission.evaluation_id == created_evaluation.id

# AND retrieving the submission should also work
retrieved = await Submission(id=created_submission.id).get_async(
synapse_client=self.syn
)
assert retrieved.id == created_submission.id

async def test_store_submission_without_entity_id_async(
self, test_evaluation: Evaluation
):
Expand Down
54 changes: 30 additions & 24 deletions tests/unit/synapseclient/models/unit_test_evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,47 +99,23 @@ def test_to_synapse_request_missing_required_fields(self):
Evaluation(
description="Test",
content_source="syn123",
submission_instructions_message="Inst",
submission_receipt_message="Rec",
),
"name",
),
(
Evaluation(
name="Test",
content_source="syn123",
submission_instructions_message="Inst",
submission_receipt_message="Rec",
),
"description",
),
(
Evaluation(
name="Test",
description="Test",
submission_instructions_message="Inst",
submission_receipt_message="Rec",
),
"content_source",
),
(
Evaluation(
name="Test",
description="Test",
content_source="syn123",
submission_receipt_message="Rec",
),
"submission_instructions_message",
),
(
Evaluation(
name="Test",
description="Test",
content_source="syn123",
submission_instructions_message="Inst",
),
"submission_receipt_message",
),
]

# Test each case
Expand All @@ -149,6 +125,36 @@ def test_to_synapse_request_missing_required_fields(self):
):
evaluation.to_synapse_request(RequestType.CREATE)

def test_to_synapse_request_without_optional_message_fields(self):
"""Test that submission_instructions_message and submission_receipt_message
are not required — the request body omits them when absent."""
Comment thread
jaymedina marked this conversation as resolved.
# GIVEN an evaluation without message fields
evaluation = Evaluation(
name="Test Evaluation",
description="This is a test evaluation",
content_source="syn123456",
)

# WHEN we generate a request body for create
request_body = evaluation.to_synapse_request(RequestType.CREATE)

# THEN the body should not include the optional message keys
assert request_body == {
"name": "Test Evaluation",
"description": "This is a test evaluation",
"contentSource": "syn123456",
}
assert "submissionInstructionsMessage" not in request_body
assert "submissionReceiptMessage" not in request_body

# AND the same applies to an update request
evaluation.id = "9614112"
evaluation.etag = "abc-123-xyz"
update_body = evaluation.to_synapse_request(RequestType.UPDATE)

assert "submissionInstructionsMessage" not in update_body
assert "submissionReceiptMessage" not in update_body

def test_set_last_persistent_instance(self):
"""Test setting the last persistent instance of an evaluation."""
# GIVEN a new evaluation
Expand Down
Loading