Skip to content
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.

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

Allow disable transaction for select populates #1067

Merged
merged 10 commits into from
Aug 29, 2024

Conversation

CBroz1
Copy link
Member

@CBroz1 CBroz1 commented Aug 14, 2024

Description

To address issues discussed in #1030, this PR allows the disabling of transaction protection.
This sacrifices full data integrity protection in order to avoid the bottleneck

Checklist:

  • No. This PR should be accompanied by a release: (yes/no/unsure)
  • N/a. If release, I have updated the CITATION.cff
  • No. This PR makes edits to table definitions: (yes/no)
  • N/a. If table edits, I have included an alter snippet for release notes.
  • No. If this PR makes changes to position, I ran the relevant tests locally.
  • I have updated the CHANGELOG.md with PR number and description.
  • I have added/edited docs/notebooks to reflect the changes

@CBroz1
Copy link
Member Author

CBroz1 commented Aug 21, 2024

This is ready for testing, but will remain a draft pending merge of #1035 and release of 0.5.3

@CBroz1 CBroz1 requested a review from samuelbray32 August 21, 2024 21:05
@CBroz1 CBroz1 marked this pull request as ready for review August 27, 2024 17:05
@CBroz1 CBroz1 mentioned this pull request Aug 27, 2024
7 tasks
else: # No transaction protection, use bare make
for key in keys:
self.make(key)
if upstream_hash != self._hash_upstream(keys):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could be made simpler for the user to clean up the mismatched hash insert by either:

  • printing the key in the raised error
  • automatically deleting the key before the error (enforces integrity)
  • pass the hash result to the make function and do the check in there before insert if there's a hash (enforces integrity, requires more edits to the code)

Copy link
Member Author

Choose a reason for hiding this comment

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

Only using the hash makes it tough to determine where the mismatch occurred. I made a commit to delete all the keys and ask the user to start over. It's not ideal, but, from the code I've seen, folks primarily run one key at a time. It seems like an unlikely case that there would be a mismatch - and that it'll have a serious impact in the timeline between now and a new DJ feature we can use

Would you rather I run a table-wise comparison across the two RestrGraph objects to nail down where the mismatch occurred?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the overly cautious solution of delete them all is fine for now. As you said, it's unlikely that it would occur, and ensuring consistency is a valid priority. User's shouldn't notice it, and if they do we would need to be going back into the code to figure out why anyways

src/spyglass/utils/dj_mixin.py Outdated Show resolved Hide resolved
src/spyglass/utils/dj_mixin.py Outdated Show resolved Hide resolved
src/spyglass/utils/dj_mixin.py Show resolved Hide resolved
@samuelbray32 samuelbray32 merged commit adfed75 into LorenFrankLab:master Aug 29, 2024
7 checks passed
@CBroz1 CBroz1 deleted the rmt branch September 5, 2024 19:23
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.

2 participants