[SYNPY-1589] Implement core functionality of Evaluation OOP model

[jaymedina]

See #1244

[jaymedina]

I'd like people's thoughts on this re-design:

Following the bug fix for update_async, I realized the way I was having users update/create their evaluations didn't match, which was confusing.

The original logic looked like this:

# Creating my first evaluation
evaluation = Evaluation(name="eval_name", description=...)
await evaluation.store_async(synapse_client=syn)

# Updating my evaluation
await evaluation.update_async(name="new_name", synapse_client=syn)

So for store_async, the logic relied on the object attributes to build and submit the request. For update_async, the logic relied on the new attributes being passed through as parameters in the method call.

I decided to consolidate this inconsistency so both update and store functionality relies on the user updating their evaluation object accordingly.

This means neither update_async and store_async require any parameters get passed to the methods themselves, rather the object itself needs to be updated. This will also help the user know what exactly their evaluation is going to look like when they make an update or store a new evaluation.

This is what it would look like irl:

In [27]: await new_evaluation.store_async(synapse_client=syn)
Out[27]: Evaluation(id='9616331', etag='8a8dbe71-5ce9-45d2-95f2-7f80987db2f7', name='ah2', description='ah', owner_id='3489628', created_on='2025-09-16T15:09:58.819Z', content_source='syn69835628', submission_instructions_message='ah', submission_receipt_message='ah')

In [28]: new_evaluation.name = 'ah2_updated'

In [29]: await new_evaluation.update_async(synapse_client=syn)
Out[29]: Evaluation(id='9616331', etag='8c027b5b-6447-4009-ba12-4a33a4d0b363', name='ah2_updated', description='ah', owner_id='3489628', created_on='2025-09-16T15:09:58.819Z', content_source='syn69835628', submission_instructions_message='ah', submission_receipt_message='ah')

The downside of this is a user would have to manually update every attribute e.g:

test_evaluation.name = new_name
test_evaluation.description = new_description
test_evaluation.submission_instructions_message = new_instructions

LMK your thoughts @BryanFauble @thomasyu888

[BryanFauble]

I don't have a strong dislike how you've split apart the create and update logic, however - If it ends up the case that these are split methods a user needs to call I would suggest that they are renamed to create_async and update_async.

However, here is something to consider. There is an alternative way to go about this which would allow you to continue using store_async for create OR update operations. If this is the route you go this is how I accomplished this for other classes.

synapsePythonClient/synapseclient/models/project.py

Lines 335 to 349 in da36c84

	if (
	self.create_or_update
	and not self._last_persistent_instance
	and (
	existing_project_id := await get_id(
	entity=self, synapse_client=synapse_client, failure_strategy=None
	)
	)
	and (
	existing_project := await Project(id=existing_project_id).get_async(
	synapse_client=synapse_client
	)
	)
	):
	merge_dataclass_entities(source=existing_project, destination=self)

To explain what is going on here:

1. The create_or_update is optional here, and is probably fine not to have for Evaluation.
2. The _last_persistent_instance is a bit of a complex concept the purpose behind this was to track if the class instance had already been read from Synapse, or stored to Synapse before during the current session. In most cases it would end up being None, but it would be filled in during the get_async operation since it was the last state that the client application knew the data to be.
3. There is a search to find the existing entity id (Evaluation ID in your case). This would either get the ID attribute if passed in OR search for it by name.
4. If an ID is found then a new get_async operation is performed to get the current state that Synapse has. After this is performed then we continue to the merge:

The merge is the important piece here, and what I think is the core problem that you're trying to solve. This merge operation allows you to perform non-destructive updates, if you implement it this would mean you could do something like:

my_eval = Evaluation(id='1234')
my_eval.description = new_description
my_eval.store()

The logic I described above would mean that ONLY the description gets updated here in a non-destructive update.

An edge case and something you might be thinking here, is "How do I delete fields/set them to None?" and that would be a great question.

The check for _last_persistent_instance means that after a resource has been read from Synapse (Or previously stored) any NEW store() operations are total replacements of the object.

That means this behaves the same as the small example I gave above:

my_eval = Evaluation(id='1234').get()
my_eval.description = new_description
my_eval.store()

But then you could also do something like:

my_eval = Evaluation(id='1234')
my_eval.description = None # Delete description
my_eval.store()

Hopefully this makes sense.

[jaymedina]

@BryanFauble thanks for the feedback - since this is the logic already in place for existing models, it makes sense for me to implement it this way too for consistency. I'm working on the refactor now and will tag you when it's finished.

[jaymedina]

@BryanFauble I've implemented the refactor in this commit: ecfc2d2

Notable details:

• I refactored and implemented merge_dataclass_entities to also support Evaluation objects, not just Synapse entities. Another major change was adding a new argument that passes a list of fields that SHOULD NOT change from the source object if a user changes them, such as things like owner_id. I'm open to discussing if this is worth it or not. The motivation behind this was to prevent users from "reusing" the Evaluation objects to interact with other Evaluation objects.
• I implemented those attributes you leverage for when/when not to trigger merge_dataclass_entities, such as _last_persistent_instance and has_changed.
• One area where I deviated from your design in the Project model is I didn't implement create_or_update. Instead, I relied on the id and _last_persistent_instance and has_changed models for when and when not to make a non-destructive change to the Synapse object. Because this is a choice that's happening "behind the curtain", I opted for adding a stdout message notifying the user when their object is going down the "create" or "update" pipeline, or not being updated at all.

Let me know your thoughts on this implementation, and if you have any other suggestions for the store_async portion of the model!

[jaymedina]

An edge case and something you might be thinking here, is "How do I delete fields/set them to None?"

All of the fields for the Evaluation model are part of the request JSON, so they have to be not-None, which was another motivation for including the new parameter fields_to_preserve_from_source in the merge utility function. If a user sets any of those specified fields to None, they should get replaced with those values which were retrieved from the last communication with Synapse so we can create a proper request JSON dictionary for the following communication.