Migrate from `upstream_hash` to tri-part `make` #1422

CBroz1 · 2025-10-03T22:53:13Z

Description

This PR replaces our upstream_hash protections with the generator-make approach in datajoint 0.14.6

Checklist:

NA. If this PR should be accompanied by a release, I have updated the CITATION.cff
NA. If this PR edits table definitions, I have included an alter snippet for release notes.
If this PR makes changes to position, I ran the relevant tests locally.
NA. If this PR makes user-facing changes, I have added/edited docs/notebooks to reflect the changes
I have updated the CHANGELOG.md with PR number and description.

codecov · 2025-10-03T23:43:21Z

Codecov Report

❌ Patch coverage is 63.68421% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.09%. Comparing base (b17c225) to head (e476f51).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/spyglass/behavior/v1/moseq.py	0.00%	23 Missing ⚠️
src/spyglass/position/v1/position_dlc_training.py	29.16%	17 Missing ⚠️
src/spyglass/common/common_ephys.py	15.78%	16 Missing ⚠️
.../spyglass/spikesorting/v0/spikesorting_curation.py	93.15%	5 Missing ⚠️
src/spyglass/spikesorting/v1/figurl_curation.py	37.50%	5 Missing ⚠️
src/spyglass/common/common_interval.py	50.00%	1 Missing ⚠️
...spyglass/spikesorting/v0/spikesorting_recording.py	91.66%	1 Missing ⚠️
...c/spyglass/spikesorting/v0/spikesorting_sorting.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1422      +/-   ##
==========================================
- Coverage   71.35%   71.09%   -0.26%     
==========================================
  Files         114      114              
  Lines       13282    13370      +88     
==========================================
+ Hits         9477     9506      +29     
- Misses       3805     3864      +59

Flag	Coverage Δ
full-tests	`71.09% <63.68%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…to 3pm

Copilot

Pull Request Overview

This PR removes the populate transaction workaround by implementing a tripartite make method pattern across multiple Spyglass tables. The changes split the traditional make method into three distinct methods: make_fetch (data retrieval), make_compute (computation), and make_insert (database insertion).

Key changes:

Removes _use_transaction class attribute and related transaction control logic from SpyglassMixin.populate()
Refactors 10+ Computed/Imported tables to use the tripartite make_fetch/make_compute/make_insert pattern
Replaces non-deterministic uuid.uuid4() calls with deterministic dj.hash.key_hash() for consistent naming

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tests/conftest.py	Disables GPU for tests via environment variable
src/spyglass/utils/dj_mixin.py	Removes transaction control logic and deprecates `use_transaction` kwarg
src/spyglass/spikesorting/v1/metric_curation.py	Refactors to tripartite make pattern
src/spyglass/spikesorting/v1/figurl_curation.py	Refactors to tripartite make pattern
src/spyglass/spikesorting/v0/spikesorting_sorting.py	Refactors to tripartite make pattern
src/spyglass/spikesorting/v0/spikesorting_recording.py	Extracts path validation to separate method
src/spyglass/spikesorting/v0/spikesorting_curation.py	Refactors multiple classes to tripartite make pattern
src/spyglass/spikesorting/v0/spikesorting_burst.py	Fixes static method call
src/spyglass/position/v1/position_dlc_training.py	Refactors to tripartite make pattern
src/spyglass/position/v1/position_dlc_project.py	Changes file_ext from enum to varchar
src/spyglass/common/common_interval.py	Adds shape check for interval conversion
src/spyglass/common/common_ephys.py	Refactors LFP table to tripartite make pattern
src/spyglass/behavior/v1/moseq.py	Refactors to tripartite make pattern
docs/src/ForDevelopers/CustomPipelines.md	Documents tripartite make pattern
docs/src/Features/Mixin.md	Removes transaction protection documentation
CHANGELOG.md	Notes infrastructure change

Comments suppressed due to low confidence (1)

src/spyglass/spikesorting/v0/spikesorting_sorting.py:307

Normal methods should have 'self', rather than 'key', as their first parameter.

    def make_insert(key, sorting_path, time_of_sort):

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/spyglass/spikesorting/v0/spikesorting_sorting.py

src/spyglass/behavior/v1/moseq.py

src/spyglass/position/v1/position_dlc_training.py

docs/src/ForDevelopers/CustomPipelines.md

src/spyglass/common/common_ephys.py

src/spyglass/spikesorting/v0/spikesorting_sorting.py

src/spyglass/spikesorting/v1/metric_curation.py

…to 3pm

Copilot

Pull Request Overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/spyglass/spikesorting/v0/spikesorting_curation.py

src/spyglass/common/common_ephys.py

src/spyglass/position/v1/position_dlc_training.py

Co-authored-by: Copilot <[email protected]>

edeno

Looks good to me. @samuelbray32 do you see anything that would prevent merging?

samuelbray32 · 2025-11-19T20:58:10Z

src/spyglass/common/common_ephys.py

        IntervalList.insert1(lfp_valid_times.as_dict, replace=True)
-        AnalysisNwbfile().log(key, table=self.full_table_name)
-        self.insert1(key)
+        self.insert1(dict(key, **added_key))


Suggested change

self.insert1(dict(key, **added_key))

AnalysisNwbfile().add(key["nwb_file_name"], lfp_file_name)

self.insert1(dict(key, **added_key))

This needs run down here now, no?

samuelbray32 · 2025-11-19T21:07:26Z

src/spyglass/utils/mixins/populate.py

                "KEY", limit=kwargs.get("limit", None)
            )

-        if use_transact is False:


I think people have custom tables that are using this feature. Do we want to give some time to migrate before removing it here?

You're right. Removing the feature was premature. Better to add deprecation warning

…to 3pm

CBroz1 added 4 commits October 3, 2025 15:03

Separate MoseqModel

6d25aa9

Fix key as sep arg

5e7090b

Separate common.LFP

01c2181

Separate FigURLCuration

2e0d382

CBroz1 added 14 commits October 14, 2025 11:53

Separate DLCModelTraining

f308b43

Separate MetricCuration

88be221

Separate Waveforms

7e5d316

Separate Waveforms 2

61168a8

Separate QualityMetrics

a669c01

Fix missing args

1b84071

Fix make_compute arg

0bdf112

Fix deterministic extractor path

7777a5f

Fix QualityMetrics, add 'self' arg

7e52bca

Separate CuratedSpikeSorting

01a1c6d

Fix CuratedSpikeSorting

b20801a

Separate SpikeSorting

6e258e2

Update changelog

b89aad9

Remove old no-transaction-make, update docs for tripart approach

4f39e29

CBroz1 requested a review from Copilot October 29, 2025 19:53

Merge branch 'master' of https://github.com/LorenFrankLab/spyglass in…

5a3eb69

…to 3pm

Copilot AI reviewed Oct 29, 2025

View reviewed changes

CBroz1 added 8 commits October 29, 2025 15:15

PR comments

803258b

Fix test

f62f036

Separate MoseqModel

93cb41a

Fix key as sep arg

b314dc0

Separate common.LFP

6d6bcc0

Separate FigURLCuration

c835802

Separate DLCModelTraining

52fb99f

Separate MetricCuration

6ce4ca7

CBroz1 added 15 commits October 29, 2025 18:14

Separate Waveforms

16c12ba

Separate Waveforms 2

c8ac376

Separate QualityMetrics

d12c290

Fix missing args

7047a5f

Fix make_compute arg

be06e46

Fix deterministic extractor path

2e71abf

Fix QualityMetrics, add 'self' arg

ccec489

Separate CuratedSpikeSorting

c1ab398

Fix CuratedSpikeSorting

e0bca4e

Separate SpikeSorting

77e964a

Update changelog

89b8d16

Remove old no-transaction-make, update docs for tripart approach

dc4e03a

PR comments

b8c864a

Fix test

dd9ae11

Merge branch '3pm' of https://github.com/cbroz1/spyglass into 3pm

dc6b17f

CBroz1 marked this pull request as ready for review October 30, 2025 03:43

Merge branch 'master' of https://github.com/LorenFrankLab/spyglass in…

ce893d6

…to 3pm

edeno requested review from Copilot, edeno and samuelbray32 November 3, 2025 17:31

Copilot AI reviewed Nov 3, 2025

View reviewed changes

src/spyglass/spikesorting/v0/spikesorting_curation.py Outdated Show resolved Hide resolved

src/spyglass/common/common_ephys.py Outdated Show resolved Hide resolved

src/spyglass/position/v1/position_dlc_training.py Outdated Show resolved Hide resolved

CBroz1 and others added 2 commits November 3, 2025 12:20

Update src/spyglass/spikesorting/v0/spikesorting_curation.py

571fa30

Co-authored-by: Copilot <[email protected]>

Remove key edits

cdf7ae2

edeno approved these changes Nov 19, 2025

View reviewed changes

samuelbray32 reviewed Nov 19, 2025

View reviewed changes

CBroz1 added 2 commits November 19, 2025 15:43

Merge branch 'master' of https://github.com/LorenFrankLab/spyglass in…

eaff799

…to 3pm

Deprecate warning for no_transaction make

e476f51

	self.insert1(dict(key, **added_key))
	AnalysisNwbfile().add(key["nwb_file_name"], lfp_file_name)
	self.insert1(dict(key, **added_key))

Migrate from upstream_hash to tri-part make #1422

Are you sure you want to change the base?

Migrate from upstream_hash to tri-part make #1422

Uh oh!

Conversation

CBroz1 commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

codecov bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edeno left a comment

Choose a reason for hiding this comment

Uh oh!

samuelbray32 Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

samuelbray32 Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

CBroz1 Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Migrate from `upstream_hash` to tri-part `make` #1422

Migrate from `upstream_hash` to tri-part `make` #1422

CBroz1 commented Oct 3, 2025 •

edited

Loading

codecov bot commented Oct 3, 2025 •

edited

Loading