Skip to content

fix(medcat-trainer): Postgres load_examples fails on unique constraint #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alhendrickson merged 1 commit into from
Jan 21, 2026
Merged

fix(medcat-trainer): Postgres load_examples fails on unique constraint #299

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion medcat-trainer/webapp/api/api/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,12 @@ def save(self, *args, skip_load=False, **kwargs):
except Exception as exc:
raise MedCATLoadException(f'Failure loading MetaCAT models - {unpacked_model_pack_path}') from exc

super().save(*args, **kwargs)
if not is_new:
Copy link
Collaborator

@mart-r mart-r Jan 21, 2026

Choose a reason for hiding this comment

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

I don't have a lot of Django knowledge.

But to me this doesn't feel quite right.

If the model is new (is_new == True) then this gets initially saved at the start of this method (line 51).
So the rest of the code should run under the assumption that if something is changed, it should be updated rather than fully re-saved.

But the current implementation performs an update if the object already existed before calling this method, but attempts to re-save the entire thing if the object was brand new. To me, the brand new behaviour would still run into the save-twice issue.

But again, not too familiar with Django. And not sure what the suitable option would / should be.

Copy link
Member

@tomolopolis tomolopolis Jan 21, 2026

Choose a reason for hiding this comment

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

ha - I should have looked up, this already is in a transaction.... in Django to set a M2M field requires the object to exist in the ORM. so Saving 'twice' is needed for new or otherwise objects

Copy link
Collaborator Author

@alhendrickson alhendrickson Jan 21, 2026

Choose a reason for hiding this comment

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

I agree with you there that it doesnt seem right

Looking at it today: I think at least this is a step forward. I think this PR fixes it for when is_new = True, and leaves the existing flow where is_new=False exactly as it is today

"To me, the brand new behaviour would still run into the save-twice issue." - with the change in this PR if is_new=True then it does an UPDATE statement not insert using this function argument update_fields=['concept_db', 'vocab']). So the is_new behavior should be fixed

For the is_new = False case, I think it's probably still broken, I think it's going to go through this function and call an INSERT, though I dont know how to make that function call

Copy link
Collaborator

@mart-r mart-r Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

You're right - I missed the not in the if statement. So the (potential) issue is (like you correctly said) with the not is_new case. The other issue is that in case of is_new the *args and **kwargs aren't being propagated. I don't know whether / what downstream effect that may or may not have. But it's certainly a diverging behaviour compared to the other calls.

Copy link
Collaborator Author

@alhendrickson alhendrickson Jan 21, 2026

Choose a reason for hiding this comment

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

I was too eager pressing the merge button.

I've added the args and kwargs on this PR #300 - good spot and I want to keep it all consistent.

super().save(*args, **kwargs)
else:
# For new objects, just update the FK fields without full save
# Fixes psycopg.errors.UniqueViolation: duplicate key value violates unique constraint "api_modelpack_pkey"
super().save(update_fields=['concept_db', 'vocab'])

def __str__(self):
return self.name
Expand Down