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

Periph table fallback on TableChain for experimenter summary #1035

Merged
merged 12 commits into from
Aug 27, 2024

Conversation

CBroz1
Copy link
Member

@CBroz1 CBroz1 commented Jul 15, 2024

Description

Thanks, @MichaelCoulter for submitting #1031

Previously, the SpyglassMixin delete process inherited banning of peripheral tables behavior, which is useful in the case of long-distance restrictions, and failed to fully inherit ignoring of non-spyglass tables. This cropped up for both (a) delete permission via the session table, and (b) downstream descendants for deleting part-masters.

I address this by giving TableChain a new fallback for the path between two nodes that includes peripheral tables, if no connection was previously found. This allows tables like IntervalLinearizedPosition to find a session connection, and ignores custom tables when checking downstream for part-master deletes.

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.
  • N/a. I have added/edited docs/notebooks to reflect the changes

@CBroz1 CBroz1 linked an issue Jul 15, 2024 that may be closed by this pull request
@CBroz1 CBroz1 marked this pull request as ready for review July 15, 2024 21:16
@edeno
Copy link
Collaborator

edeno commented Jul 17, 2024

@MichaelCoulter did this solution work for you?

@samuelbray32
Copy link
Collaborator

Was trying to test with data, but ran into issues from legacy tables in our database (#976)

@CBroz1
Copy link
Member Author

CBroz1 commented Jul 17, 2024

Was trying to test with data, but ran into issues from legacy tables in our database (#976)

I'll see if I can find a systematic way to get rid of the empty legacy tables

@samuelbray32
Copy link
Collaborator

Thanks @CBroz1, as a note, the blocking ones I was hitting had small (~1 entries) likely used in development, but not definitively named as such

@samuelbray32
Copy link
Collaborator

@CBroz1 , wanted to know if you think this is a reasonable feature to include in this PR: I found a way to load all spyglass shared schemas such that their nodes are in the dependency graph.

import datajoint as dj
from spyglass.utils.database_settings import SHARED_MODULES

def load_spyglass_shared_schemas():
    # Get a list of all shared schemas in spyglass
    keys = dj.conn().query(
        """
                    SELECT
                        concat('`', table_schema, '`.`', table_name, '`') as tab, column_name
                    FROM information_schema.key_column_usage
                    WHERE table_name not LIKE "~%%" AND constraint_name="PRIMARY"
                    """
    )

    shared_schema = []
    for key in keys:
        schema_name = key[0].split(".")[0].strip("`")
        schema_prefix = schema_name.split("_")[0]
        if schema_prefix in SHARED_MODULES and schema_name not in shared_schema:
            shared_schema.append(schema_name)
            
    # Load the dependencies for all shared schemas
    for s in shared_schema:
        dj.schema(s).connection.dependencies.load()

    return shared_schema

load_spyglass_shared_schemas()
  • Running this before constructing the RestrGraph allowed things to run through without importing specific shared tables. Overhead on run time was about ~5 seconds for this function, but it saved time in the delete call since dependencies were already loaded.

  • As implemented this looks for schema with prefixes contained in SHARED_MODULES. Could extend to allow users to import schemas with their prefix into the graph easily

  • Running this here could be an simple way to reduce need for imports

Copy link
Collaborator

@samuelbray32 samuelbray32 left a comment

Choose a reason for hiding this comment

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

Could include comment above, otherwise looks and runs fine

@samuelbray32 samuelbray32 merged commit ecf468e into LorenFrankLab:master Aug 27, 2024
7 checks passed
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.

ValueError on non-spy table for _get_exp_summary()
3 participants