[SYNPY-1590] Implement core functionality of Submission(+Status, +Bundle) OOP model #1251

jaymedina · 2025-09-30T15:34:17Z

All tests passing:

(synapseclient) bash-3.2$ pytest tests/integration/synapseclient/models/synchronous/*submission*.py 
======================================================================= test session starts ========================================================================
platform darwin -- Python 3.9.18, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/jmedina/Documents/forks/synapsePythonClient
configfile: pytest.ini
plugins: asyncio-0.24.0, socket-0.7.0, anyio-4.6.0, mock-3.14.0
asyncio: mode=auto, default_loop_scope=session
collected 67 items                                                                                                                                                 

tests/integration/synapseclient/models/synchronous/test_submission.py .....................                                                                  [ 31%]
tests/integration/synapseclient/models/synchronous/test_submission_bundle.py ...............                                                                 [ 53%]
tests/integration/synapseclient/models/synchronous/test_submission_status.py .......................                                                         [ 88%]
tests/integration/synapseclient/models/synchronous/test_submissionview.py ........                                                                           [100%]

======================================================================= test session starts ========================================================================
platform darwin -- Python 3.9.18, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/jmedina/Documents/forks/synapsePythonClient
configfile: pytest.ini
plugins: asyncio-0.24.0, socket-0.7.0, anyio-4.6.0, mock-3.14.0
asyncio: mode=auto, default_loop_scope=session
collected 68 items                                                                                                                                                 

tests/integration/synapseclient/models/async/test_submission_async.py ......................                                                                 [ 32%]
tests/integration/synapseclient/models/async/test_submission_bundle_async.py ...............                                                                 [ 54%]
tests/integration/synapseclient/models/async/test_submission_status_async.py .......................                                                         [ 88%]

======================================================================= test session starts ========================================================================
platform darwin -- Python 3.9.18, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/jmedina/Documents/forks/synapsePythonClient
configfile: pytest.ini
plugins: asyncio-0.24.0, socket-0.7.0, anyio-4.6.0, mock-3.14.0
asyncio: mode=auto, default_loop_scope=session
collected 53 items                                                                                                                                                 

tests/unit/synapseclient/models/async/unit_test_submission_async.py ................                                                                         [ 30%]
tests/unit/synapseclient/models/async/unit_test_submission_bundle_async.py ..............                                                                    [ 56%]
tests/unit/synapseclient/models/async/unit_test_submission_status_async.py .......................                                                           [100%]

======================================================================== 53 passed in 1.61s ========================================================================

======================================================================= test session starts ========================================================================
platform darwin -- Python 3.9.18, pytest-8.3.3, pluggy-1.5.0
rootdir: /Users/jmedina/Documents/forks/synapsePythonClient
configfile: pytest.ini
plugins: asyncio-0.24.0, socket-0.7.0, anyio-4.6.0, mock-3.14.0
asyncio: mode=auto, default_loop_scope=session
collected 67 items                                                                                                                                                 

tests/unit/synapseclient/models/synchronous/unit_test_submission.py .............................                                                            [ 43%]
tests/unit/synapseclient/models/synchronous/unit_test_submission_bundle.py ...............                                                                   [ 65%]
tests/unit/synapseclient/models/synchronous/unit_test_submission_status.py .......................                                                           [100%]

synapseclient/models/submission_status.py

synapseclient/models/submission.py

…issionstatus file

…rn as evaluations design

…OR submission annotations

… organizer on the client. cancel request returns no response body.

…on_number can be ignored for docker submissions

thomasyu888 · 2025-12-01T22:51:39Z

synapseclient/annotations.py

+    etag: str,
+    annotations: typing.Dict[str, typing.Any],
+    logger: typing.Optional[typing.Any] = None,
+) -> typing.Dict[str, typing.Any]:


Discussion: Should SubmissionAnnotations be a class structure or is that not needed?

I think we already have an annotation class that can be used here: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/models/annotations.py#L18

andrewelamb · 2025-12-02T15:34:33Z

synapseclient/api/evaluation_services.py

+        etag: The current eTag of the Entity being submitted.
+        synapse_client: If not passed in and caching was not disabled by `Synapse.allow_client_caching(False)` this will use the last created
+                        instance from the Synapse class constructor.
+    """


Missing a description of the return object

synapseclient/api/evaluation_services.py

andrewelamb · 2025-12-02T15:42:17Z

synapseclient/api/evaluation_services.py

+        The caller must be granted the ACCESS_TYPE.READ on the specified Evaluation.
+        Furthermore, the caller must be granted the ACCESS_TYPE.READ_PRIVATE_SUBMISSION
+        to see all data marked as "private" in the SubmissionStatus.
+    """


What happens if the caller has ACCESS_TYPE.READ, but not ACCESS_TYPE.READ_PRIVATE_SUBMISSION permissions?

synapseclient/api/evaluation_services.py

andrewelamb · 2025-12-02T16:05:08Z

synapseclient/models/submission.py

+            return entity_info
+        except Exception as e:
+            raise ValueError(
+                f"Unable to fetch entity information for {self.entity_id}: {e}"


Any reason not to do
raise ValueError(msg) from e ?

andrewelamb · 2025-12-02T16:07:25Z

synapseclient/models/submission.py

@@ -0,0 +1,920 @@
+from dataclasses import dataclass, field
+from typing import AsyncGenerator, Dict, Generator, List, Optional, Protocol, Union


You don't need to import Dict or List types anymore. Instead, use the lower case version type hints dict and list.

andrewelamb · 2025-12-02T16:08:54Z

synapseclient/models/submission.py

+
+        Returns:
+            Dictionary containing entity information from the REST API.
+


Since this is a user-facing method, should the dictionary returned be described?

andrewelamb · 2025-12-02T16:09:45Z

synapseclient/models/submission.py

+                docker_tag_response = await client.rest_get_async(
+                    f"/entity/{self.entity_id}/dockerTag"
+                )
+


Should this be an api function?

andrewelamb · 2025-12-02T16:21:35Z

synapseclient/models/submission.py

+            request_body["dockerRepositoryName"] = self.docker_repository_name
+        if self.docker_digest is not None:
+            request_body["dockerDigest"] = self.docker_digest
+


Should we have a list of optional fields and then use a for loop to iterate over them, and them to the request body if they are not none?

andrewelamb · 2025-12-02T16:23:37Z

synapseclient/models/submission.py

+            "entityId": self.entity_id,
+            "evaluationId": self.evaluation_id,
+            "versionNumber": self.version_number,
+        }


Should self.contributors be here as well? It doesn't appear to be optional.

andrewelamb · 2025-12-02T16:25:06Z

synapseclient/models/submission.py

+
+        Raises:
+            ValueError: If any required attributes are missing.
+        """


Should there be an example for this method?

andrewelamb · 2025-12-02T16:28:12Z

synapseclient/models/submission.py

+            ```
+        """
+
+        if self.entity_id:


I would structure this as:

if not self.entity_id: raiseValueError(msg) <code that is in current if block>

This reduces the amount of indented code.

andrewelamb · 2025-12-02T16:31:19Z

synapseclient/models/submission.py

+
+        if not self.entity_etag:
+            raise ValueError("Unable to fetch etag for entity")
+


Can you move this up to right after line 582 where you get the entity tag? That way it would fail faster, and when there wasn't an entity tag, you wouldn't have to the docker checking.

andrewelamb · 2025-12-02T17:11:04Z

synapseclient/models/submission_status.py

+
+        Returns:
+            A dictionary containing the request body for updating a submission status.
+


andrewelamb · 2025-12-02T17:11:45Z

synapseclient/models/submission_status.py

+            request_body["canCancel"] = self.can_cancel
+        if self.cancel_requested is not None:
+            request_body["cancelRequested"] = self.cancel_requested
+


Same suggestion as Submission about turning this into a for loop.

andrewelamb · 2025-12-02T17:19:23Z

synapseclient/models/submission_status.py

+            submission_status._set_last_persistent_instance()
+            submission_statuses.append(submission_status)
+
+        return submission_statuses


Should this be a generator method?

SageGJ · 2025-12-02T16:32:21Z

synapseclient/annotations.py



+def to_submission_annotations(
+    id: typing.Union[str, int],


Nit: wouldn't it be less verbose to import the classes from typing that we'll need at the beginning of the file instead of accessing them through the library each time?

SageGJ · 2025-12-02T16:33:38Z

synapseclient/annotations.py

+    id: typing.Union[str, int],
+    etag: str,
+    annotations: typing.Dict[str, typing.Any],
+    logger: typing.Optional[typing.Any] = None,


should this really be of type Any instead of a union between logger.Logger and the synapse client logger?

SageGJ · 2025-12-02T16:36:47Z

synapseclient/models/submission.py

+            Submission objects as they are retrieved from the API.
+
+        Example: Getting submissions for an evaluation
+            &nbsp;


what is &nbsp?

This adds non-breaking space in the documentation. We previously had issues where the docs didn’t render correctly in the UI after deployment, and this change fixes that.

SageGJ · 2025-12-02T16:39:29Z

synapseclient/models/submission.py

+        except Exception as e:
+            raise ValueError(
+                f"Unable to fetch entity information for {self.entity_id}: {e}"
+            )


Should we be catching every possible exception here and assuming they're all the same cause, or would it be useful to break the catching up into synapse errors and other possible kinds? Also would a LookupError make more sense here than a ValueError?

Alternatively, we could do raise ValueError(msg) from e here. Then the user would still get a ValueError but would also see the exception that was caught as well.

SageGJ · 2025-12-02T16:43:33Z

tests/integration/synapseclient/models/async/test_submission_async.py

+    # async def test_store_submission_with_docker_repository_async(
+    #     self, test_evaluation: Evaluation
+    # ):
+    #     # GIVEN we would need a Docker repository entity (mocked for this test)
+    #     # This test demonstrates the expected behavior for Docker repository submissions
+
+    #     # WHEN I create a submission for a Docker repository entity using async method
+    #     # TODO: This would require a real Docker repository entity in a full integration test
+    #     submission = Submission(
+    #         entity_id="syn123456789",  # Would be a Docker repository ID
+    #         evaluation_id=test_evaluation.id,
+    #         name=f"Docker Submission {uuid.uuid4()}",
+    #     )
+
+    #     # THEN the submission should handle Docker-specific attributes
+    #     # (This test would need to be expanded with actual Docker repository setup)
+    #     assert submission.entity_id == "syn123456789"
+    #     assert submission.evaluation_id == test_evaluation.id


Can this be removed?

SageGJ · 2025-12-02T16:50:21Z

tests/integration/synapseclient/models/async/test_submission_status_async.py

+    ):
+        """Test that storing a submission status without changes shows warning async."""
+        # GIVEN a submission status that hasn't been modified
+        # (it already has _last_persistent_instance set from get())


would it make sense to assert that _last_persistent_instance is some specific value or at least exists since we're depending on that happening outside this specific test?

SageGJ · 2025-12-02T16:52:53Z

tests/integration/synapseclient/models/async/test_submission_status_async.py

+
+        # THEN the batch update should succeed
+        assert response is not None
+        assert "batchToken" in response or response == {}  # Response format may vary


what causes the response formats to vary? should those be different parametrizations for this test?

SageGJ · 2025-12-02T16:54:34Z

tests/integration/synapseclient/models/synchronous/test_submission.py

+        # This test demonstrates the expected behavior for Docker repository submissions
+
+        # WHEN I create a submission for a Docker repository entity
+        # TODO: This would require a real Docker repository entity in a full integration test


Has a JIRA ticket been created for this TODO? if so can we link to it?

SageGJ · 2025-12-02T16:55:54Z

tests/integration/synapseclient/models/synchronous/test_submission.py

+    # TODO: Add with SubmissionStatus model tests
+    # async def test_cancel_submission_successfully(
+    #     self, test_evaluation: Evaluation, test_file: File
+    # ):
+    #     # GIVEN a submission
+    #     submission = Submission(
+    #         entity_id=test_file.id,
+    #         evaluation_id=test_evaluation.id,
+    #         name=f"Test Submission for Cancellation {uuid.uuid4()}",
+    #     )
+    #     created_submission = submission.store(synapse_client=self.syn)
+    #     self.schedule_for_cleanup(created_submission.id)
+
+    #     # WHEN I cancel the submission
+    #     cancelled_submission = created_submission.cancel(synapse_client=self.syn)
+
+    #     # THEN the submission should be cancelled
+    #     assert cancelled_submission.id == created_submission.id


Would it be better to remove this commented out section and create a JIRA issue so that we can track and prioritize?

SageGJ · 2025-12-02T16:57:08Z

tests/integration/synapseclient/models/synchronous/test_submission_status.py

+        assert (
+            submission_status.status is not None
+        )  # Should have some status (e.g., "RECEIVED")


same comment as similar one above

andrewelamb · 2025-12-02T17:30:32Z

synapseclient/annotations.py

+                client.logger.warning(
+                    f"Annotation '{key}' has an empty value list and will be skipped"
+                )
+            continue


Isn't this continue redundant, since it's at the bottom of the loop?

andrewelamb · 2025-12-02T17:35:58Z

tests/integration/synapseclient/models/async/test_submission_async.py

@@ -1,199 +1,674 @@
-def test_create_submission_async():


Is this file testing the non-OOP code? I'm assuming that code is deprecated and hasn't been removed yet. If so, should these tests remain until it is sunsetted?

andrewelamb · 2025-12-02T17:38:51Z

Looking good so far! I didn't see any deprecations, will you be doing that as part of this PR?

…ssion-model-functionality

jaymedina · 2025-12-03T20:39:07Z

synapseclient/models/submission_status.py

+            # evaluation_id is required when annotations are provided for scopeId
+            if self.evaluation_id is None:
+                raise ValueError(
+                    "Your submission status object is missing the 'evaluation_id' attribute. This attribute is required when submissions are updated with annotations. Please retrieve your submission status with .get() to populate this field."


@andrewelamb thanks for bringing up this question because it exposed a flaw in the current logic for SubmissionStatus.

A while ago on Slack, we had this conversation on the differences between the annotations object and the new submission_annotations object.

Long story short:

annotations is the legacy object used when Submission Views didn't exist yet, and CnB was using the leaderboard wiki

submission_annotations is the new annotations object, and these annotation types show up in submission views

I agreed with @BryanFauble in wanting to support both annotation types because annotations has not yet been sunsetted.

The catch is this annotation type requires an evaluation_id attribute to be populated, which complicates things a bit because this is not an attribute that's part of the SubmissionStatus response object (🤷‍♀️). But it is part of the Submission response object, so I did a little hacky thing and called the get_submission function which exposes the Evaluation service that retrieves a Submission response object, grabbed the evaluation ID from there, and assigned it as an attribute of the Submission_Status_ object.

This all works fine if the user has READ_PRIVATE_SUBMISSION access and beyond, but not if they only have READ access. If they don't have READ_PRIVATE_SUBMISSION access, they can't grab any submission (not even their own - see documentation), and the call for the submission status will lead to a "not enough permissions" failure, even though they technically have the right permissions to get the status but not the submission.

The user should safely assume they don't need more than READ access to grab the submission status, so I'll have to remove this logic.

Possible ways forward:

Keep supporting the legacy annotations, but require the user to manually populate the evaluation_id attribute when necessary (i.e. when storing their SubmissionStatus with a populated annotations attribute). This is pretty straightforward to implement, except it's confusing from a user-perspective because it now involves supporting an attribute (evaluation_id) that you need in some situations and not others.

Get rid of support for legacy annotations, and the evaluation_id attribute.

I would love some input on this. Right now I lean towards 2.

For @vpchung:

The tldr is that supporting the legacy annotations object has turned out to be more of a headache than anticipated, because of the scopeId (AKA evaluation ID) attribute and the different permission requirements between getting a Submission and getting a SubmissionStatus from Synapse. Which of the 2 possible ways forward listed above do you prefer?

side note: I have some lingering questions for the platform team on why some things were designed the way they are, which I've left on Slack here

I would keep vote to keep supporting the annotations object until v5.0. But:

If it hasn't yet, deprecate it with a warning to start using the new object ASAP.

Raise an exception when the user only has READ_PRIVATE_SUBMISSION permissions that:

That they don't have the correct permissions

That they should start using the new object ASAP

As a work-around they can supply the evaluation_id (is that correct?)

* always log warning * check assert called with * patch object on class instance * Revert "always log warning" This reverts commit ae63c81. * update patch target * re-add erroneously removed line

jaymedina mentioned this pull request Oct 9, 2025

[SYNPY-1590] Implement Submission(+Status, +Bundle) OOP model #1250

Draft

15 tasks

jaymedina force-pushed the synpy-1590-submission-model-main branch from 8b50d06 to a061fb4 Compare November 5, 2025 14:46

jaymedina force-pushed the synpy-1590-submission-model-functionality branch from 62ce5fb to f0d7ecc Compare November 5, 2025 15:04

jaymedina commented Nov 12, 2025

View reviewed changes

synapseclient/models/submission_status.py Show resolved Hide resolved

jaymedina commented Nov 21, 2025

View reviewed changes

synapseclient/models/submission.py Show resolved Hide resolved

jaymedina marked this pull request as ready for review November 21, 2025 19:29

jaymedina requested a review from a team as a code owner November 21, 2025 19:29

tdd initial tests

c748132

jaymedina force-pushed the synpy-1590-submission-model-main branch from a061fb4 to c748132 Compare December 1, 2025 16:33

jaymedina added 21 commits December 1, 2025 11:35

initial intro of dataclasses

33643ae

expose api services for submission object

8a074e6

style and update docstring

c97b891

add submission and submissionstatus models

f79f683

add submission status retrieval and update methods; remove empty subm…

99ebaa1

…issionstatus file

pipe query params directly into restAPI httpx requests

33c651f

new dataclass object submission_bundle

ae05b91

move submission services functions to evaluation_services.py

f51188d

renaming imports, to_synapse_request, request body refactor

33f7a6a

patching up store method signature

4a91a2e

update docs

65eac22

new suite of tests

a5ec33a

submissionstatus rework as a mutable object

a160585

bug fix for Statuses: updated to_synapse_request to follow same patte…

660259e

…rn as evaluations design

replace != with is not for full object comparison (not just keys)

55a229c

expose the is_private arg for to_submission_status_annotations ONLY F…

3479046

…OR submission annotations

fixed submission status/submission annotations

896a3c1

add support for legacy annotations

26a02f1

remove debug prints

271181a

get_all_submission_statuses now returns a list of substat objects

c8c1041

docstring updates

0436931

jaymedina added 12 commits December 1, 2025 11:35

fix submissionStatus integ tests and has_changed attribute

9c2c0d1

new substatus async integ tests. can_cancel can now be modified by an…

aee4ccd

… organizer on the client. cancel request returns no response body.

new test class for submission cancel functionality

4a5fe53

substatus async unit tests

480d5de

remove compare=false for some attributes. update sync unit tests

4ca53e0

add submissionBundle integration tests

3f25df8

add submissionBundle unit tests

b5839c3

remove unnecessary imports and add style

5526b8d

get_evaluation_submissions returns generator object

bdeaaa7

get_user_submissions returns generator object

7add157

submissionBundle methods return generators

35696eb

address final todo: implement docker_tag, and note in docs that versi…

b8c0ee5

…on_number can be ignored for docker submissions

jaymedina force-pushed the synpy-1590-submission-model-functionality branch from a9977b2 to b8c0ee5 Compare December 1, 2025 16:35

jaymedina added 2 commits December 1, 2025 13:28

import -> imports

73c5f1a

change back to import. patch uses synapse logger instance.

8db9e67

thomasyu888 requested a review from andrewelamb December 1, 2025 22:48

thomasyu888 reviewed Dec 1, 2025

View reviewed changes

andrewelamb reviewed Dec 2, 2025

View reviewed changes

SageGJ reviewed Dec 2, 2025

View reviewed changes

andrewelamb reviewed Dec 2, 2025

View reviewed changes

lock mkdocstrings-python to >=2.0.0

53d9852

jaymedina force-pushed the synpy-1590-submission-model-main branch from c748132 to 31d9583 Compare December 2, 2025 19:47

jaymedina and others added 2 commits December 2, 2025 14:55

Merge branch 'synpy-1590-submission-model-main' into synpy-1590-submi…

1561141

…ssion-model-functionality

add Return description to create_submission

4d082c3

SageGJ mentioned this pull request Dec 2, 2025

[SYNPY-1714] Fix Issues with Failing Tests #1287

Merged

jaymedina commented Dec 3, 2025

View reviewed changes

[SYNPY-1714] Fix Issues with Failing Tests (#1287)

2272d47

* always log warning * check assert called with * patch object on class instance * Revert "always log warning" This reverts commit ae63c81. * update patch target * re-add erroneously removed line

		@@ -0,0 +1,920 @@
		from dataclasses import dataclass, field
		from typing import AsyncGenerator, Dict, Generator, List, Optional, Protocol, Union


		Returns:
		Dictionary containing entity information from the REST API.


		if not self.entity_etag:
		raise ValueError("Unable to fetch etag for entity")


		Returns:
		A dictionary containing the request body for updating a submission status.

[SYNPY-1590] Implement core functionality of Submission(+Status, +Bundle) OOP model #1251

Are you sure you want to change the base?

[SYNPY-1590] Implement core functionality of Submission(+Status, +Bundle) OOP model #1251

Uh oh!

Conversation

jaymedina commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasyu888 Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewelamb commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymedina commented Sep 30, 2025 •

edited

Loading

thomasyu888 Dec 1, 2025 •

edited

Loading