Skip to content

Conversation

@jacobtylerwalls
Copy link
Member

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Bulk save graph entities.

I had this lying around from debugging #12088 earlier this year, but I think it's still useful.

Checklist

  • I targeted one of these branches:
    • dev/8.1.x (under development): features, bugfixes not covered below
    • dev/8.0.x (main support): regressions, crashing bugs, security issues, major bugs in new features
    • dev/7.6.x (extended support): major security issues, data loss issues
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

mind resolving the merge conflicts?

@jacobtylerwalls jacobtylerwalls force-pushed the jtw/bulk-save-graph-entities branch from 9d58fa4 to e588c0c Compare August 16, 2025 21:11
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

won't this skip any functionality that runs in the save and delete methods?

@jacobtylerwalls
Copy link
Member Author

Yep, one outcome of this conversation could be "don't do this", but then I was hoping to capture the "why not" in a test case that would fail if this is attempted in the future.

I just took a look: the only side effects in the save() methods in question here involve:

  • minting new node aliases
  • nulling the source_identifier field if the pk matches the source_identifier

I can update this PR to handle those cases. More interesting is what about the future case?


Make of this what you will, but I think there are several places where Arches is suffering from the lack of a form/serializer layer at the perimeter to validate inputs and coerce types. One angle is #12142, there are others. I totally understand why there is code to mint aliases and support graph versioning in the model save methods, but it feels like a compromise given the general lack of a form/serializer layer. To me, saying we shouldn't bulk save because we need to preserve the coupling of cleaning data to saving objects individually seems like memorializing the wrong design. I think instead we should encourage liberating side effects out of the model save and delete methods to make the models more bulk-friendly.


I could get you a little profile of the before & after performance impact here to help with a decision, just wanted to give you a frame to react to first!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants