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

Error in LFPOutput due to old parts #815

Closed
samuelbray32 opened this issue Feb 1, 2024 · 21 comments
Closed

Error in LFPOutput due to old parts #815

samuelbray32 opened this issue Feb 1, 2024 · 21 comments
Assignees
Labels
bug Something isn't working merge To do with merge tables

Comments

@samuelbray32
Copy link
Collaborator

Describe the bug

  • Error originated when trying to call LFPOutput().fetch_nwb(restriction={'merge_id':'001d08a1-5365-3c20-716b-f431ab136a55'})
  • Tracing through the error stack it stems from a failure to define LFPOutput().source_class_dict()
  • Error stems because ImportedLFPV1 Is still in LFPOutput.parts even thoug it was removed from the codebase ~8months ago.

My guess is this is just an error in the franklab database. May stem from #782 which had changes to Merge.fetch_nwb() May apply to other merge tables as well?

To Reproduce

from spyglass.lfp import LFPOutput
LFPOutput().source_class_dict

Error

{

"name": "AttributeError",
"message": "module 'spyglass.lfp.lfp_merge' has no attribute 'ImportedLFPV1'",
"stack": "---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[11], line 12
4 # LFPOutput().fetch_nwb(restriction={'merge_id':'001d08a1-5365-3c20-716b-f431ab136a55'})
5
6 # LFPOutput().merge_restrict_class({'merge_id':'001d08a1-5365-3c20-716b-f431ab136a55'})
(...)
9
10 # print(LFPOutput().parts(camel_case=True))#source_class_dict()
11 LFPOutput()
---> 12 LFPOutput().source_class_dict

File ~/Documents/spyglass/src/spyglass/utils/dj_merge_tables.py:666, in Merge.source_class_dict(self)
664 if not self._source_class_dict:
665 module = getmodule(self)
--> 666 self._source_class_dict = {
667 part_name: getattr(module, part_name)
668 for part_name in self.parts(camel_case=True)
669 }
670 return self._source_class_dict

File ~/Documents/spyglass/src/spyglass/utils/dj_merge_tables.py:667, in (.0)
664 if not self._source_class_dict:
665 module = getmodule(self)
666 self._source_class_dict = {
--> 667 part_name: getattr(module, part_name)
668 for part_name in self.parts(camel_case=True)
669 }
670 return self._source_class_dict

AttributeError: module 'spyglass.lfp.lfp_merge' has no attribute 'ImportedLFPV1'"
}

@samuelbray32 samuelbray32 added bug Something isn't working merge To do with merge tables labels Feb 1, 2024
@CBroz1
Copy link
Member

CBroz1 commented Feb 1, 2024

It sounds like this is solvable by

deprecated_table = dj.FreeTable(dj.conn(), '`lfp_merge`.`l_f_p_output__imported_l_f_p_v1`')
deprecated_table.drop()

@edeno - Can you see any reason not to do so?

@edeno
Copy link
Collaborator

edeno commented Feb 1, 2024

Seems fine with me

@CBroz1
Copy link
Member

CBroz1 commented Feb 1, 2024

This table has been dropped. If a similar error occurs elsewhere, the drop_quick() method should be used. See datajoint-python #374

@CBroz1 CBroz1 closed this as completed Feb 1, 2024
@xlsun79
Copy link
Contributor

xlsun79 commented Mar 28, 2024

I ran into this attribute error as well when trying to populate the LFPBand table using the LFPOutput table. I did the drop_quick command as @CBroz1 suggested, but then got another error:

Error
---------------------------------------------------------------------------
DataJointError                            Traceback (most recent call last)
Cell In [37], line 1
----> 1 lfp_band.LFPBandV1().populate(lfp_band.LFPBandSelection() & lfp_band_key)
      2 lfp_band.LFPBandV1()

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/autopopulate.py:230, in AutoPopulate.populate(self, suppress_errors, return_exception_objects, reserve_jobs, order, limit, max_calls, display_progress, processes, make_kwargs, *restrictions)
    226 if processes == 1:
    227     for key in (
    228         tqdm(keys, desc=self.__class__.__name__) if display_progress else keys
    229     ):
--> 230         error = self._populate1(key, jobs, **populate_kwargs)
    231         if error is not None:
    232             error_list.append(error)

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/autopopulate.py:281, in AutoPopulate._populate1(self, key, jobs, suppress_errors, return_exception_objects, make_kwargs)
    279 self.__class__._allow_insert = True
    280 try:
--> 281     make(dict(key), **(make_kwargs or {}))
    282 except (KeyboardInterrupt, SystemExit, Exception) as error:
    283     try:

File ~/code/spyglass/src/spyglass/lfp/analysis/v1/lfp_band.py:179, in LFPBandV1.make(self, key)
    176 def make(self, key):
    177     # get the NWB object with the lfp data; FIX: change to fetch with additional infrastructure
    178     lfp_key = {"merge_id": key["lfp_merge_id"]}
--> 179     lfp_object = (LFPOutput & lfp_key).fetch_nwb()[0]["lfp"]
    181     # get the electrodes to be filtered and their references
    182     lfp_band_elect_id, lfp_band_ref_id = (
    183         LFPBandSelection().LFPBandElectrode() & key
    184     ).fetch("electrode_id", "reference_elect_id")

File ~/code/spyglass/src/spyglass/utils/dj_merge_tables.py:520, in Merge.fetch_nwb(self, restriction, multi_source, disable_warning, *attrs, **kwargs)
    517     raise ValueError("Try replacing Merge.method with Merge().method")
    518 restriction = restriction or self.restriction or True
--> 520 return self.merge_restrict_class(restriction).fetch_nwb()

File ~/code/spyglass/src/spyglass/utils/dj_merge_tables.py:710, in Merge.merge_restrict_class(self, key)
    708 def merge_restrict_class(self, key: dict) -> dj.Table:
    709     """Returns native parent class, restricted with key."""
--> 710     parent_key = self.merge_get_parent(key).fetch("KEY", as_dict=True)
    712     if len(parent_key) > 1:
    713         raise ValueError(
    714             f"Ambiguous entry. Data has mult rows in parent:\n\tData:{key}"
    715             + f"\n\t{parent_key}"
    716         )

File ~/code/spyglass/src/spyglass/utils/dj_merge_tables.py:634, in Merge.merge_get_parent(cls, restriction, join_master, multi_source, return_empties, add_invalid_restrict)
    599 @classmethod
    600 def merge_get_parent(
    601     cls,
   (...)
    606     add_invalid_restrict: bool = True,
    607 ) -> dj.FreeTable:
    608     """Returns a list of part parents with restrictions applied.
    609 
    610     Rather than part tables, we look at parents of those parts, the source
   (...)
    631         Parent of parts of Merge Table as FreeTable.
    632     """
--> 634     part_parents = cls._merge_restrict_parents(
    635         restriction=restriction,
    636         as_objects=True,
    637         return_empties=return_empties,
    638         add_invalid_restrict=add_invalid_restrict,
    639     )
    641     if not multi_source and len(part_parents) != 1:
    642         raise ValueError(
    643             f"Found  {len(part_parents)} potential parents: {part_parents}"
    644             + "\n\tTry adding a string restriction when invoking "
    645             + "`get_parent`. Or permitting multiple sources with "
    646             + "`multi_source=True`."
    647         )

File ~/code/spyglass/src/spyglass/utils/dj_merge_tables.py:211, in Merge._merge_restrict_parents(cls, restriction, parent_name, as_objects, return_empties, add_invalid_restrict)
    182 """Returns a list of part parents with restrictions applied.
    183 
    184 Rather than part tables, we look at parents of those parts, the source
   (...)
    204     list of datajoint tables, parents of parts of Merge Table
    205 """
    206 # .restrict(restriction) does not work on returned part FreeTable
    207 # & part.fetch below restricts parent to entries in merge table
    208 part_parents = [
    209     parent
    210     & part.fetch(*part.heading.secondary_attributes, as_dict=True)
--> 211     for part in cls()._merge_restrict_parts(
    212         restriction=restriction,
    213         return_empties=return_empties,
    214         add_invalid_restrict=add_invalid_restrict,
    215     )
    216     for parent in part.parents(as_objects=True)  # ID respective parents
    217     if cls().table_name not in parent.full_table_name  # Not merge table
    218 ]
    219 if parent_name:
    220     part_parents = [
    221         p
    222         for p in part_parents
    223         if from_camel_case(parent_name) in p.full_table_name
    224     ]

File ~/code/spyglass/src/spyglass/utils/dj_merge_tables.py:167, in Merge._merge_restrict_parts(cls, restriction, as_objects, return_empties, add_invalid_restrict)
    164             parts.append(part)
    166 if not return_empties:
--> 167     parts = [p for p in parts if len(p)]
    168 if not as_objects:
    169     parts = [p.full_table_name for p in parts]

File ~/code/spyglass/src/spyglass/utils/dj_merge_tables.py:167, in <listcomp>(.0)
    164             parts.append(part)
    166 if not return_empties:
--> 167     parts = [p for p in parts if len(p)]
    168 if not as_objects:
    169     parts = [p.full_table_name for p in parts]

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/expression.py:543, in QueryExpression.__len__(self)
    534 def __len__(self):
    535     """:return: number of elements in the result set e.g. ``len(q1)``."""
    536     return self.connection.query(
    537         "SELECT {select_} FROM {from_}{where}".format(
    538             select_=(
    539                 "count(*)"
    540                 if any(self._left)
    541                 else "count(DISTINCT {fields})".format(
    542                     fields=self.heading.as_sql(
--> 543                         self.primary_key, include_aliases=False
    544                     )
    545                 )
    546             ),
    547             from_=self.from_clause(),
    548             where=self.where_clause(),
    549         )
    550     ).fetchone()[0]

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/expression.py:96, in QueryExpression.primary_key(self)
     94 @property
     95 def primary_key(self):
---> 96     return self.heading.primary_key

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/heading.py:130, in Heading.primary_key(self)
    128 @property
    129 def primary_key(self):
--> 130     return [k for k, v in self.attributes.items() if v.in_key]

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/heading.py:121, in Heading.attributes(self)
    118 @property
    119 def attributes(self):
    120     if self._attributes is None:
--> 121         self._init_from_database()  # lazy loading from database
    122     return self._attributes

File ~/anaconda3/envs/spyglass/lib/python3.9/site-packages/datajoint/heading.py:220, in Heading._init_from_database(self)
    218         logger.warning("Could not create the ~log table")
    219         return
--> 220     raise DataJointError(
    221         "The table `{database}`.`{table_name}` is not defined.".format(
    222             table_name=table_name, database=database
    223         )
    224     )
    225 self._table_status = {k.lower(): v for k, v in info.items()}
    226 cur = conn.query(
    227     "SHOW FULL COLUMNS FROM `{table_name}` IN `{database}`".format(
    228         table_name=table_name, database=database
    229     ),
    230     as_dict=True,
    231 )

DataJointError: The table `lfp_merge`.`l_f_p_output__imported_l_f_p_v1` is not defined.
Wondering why it waas still looking for this deprecated table after I dropped it. I git pulled spyglass last night so it shouldn't be a version issue. Thanks for any insight!

@samuelbray32
Copy link
Collaborator Author

Can you give an example of a key your were trying that caused the error?

@xlsun79
Copy link
Contributor

xlsun79 commented Mar 28, 2024

Yup! Just using “nwb_file_name”: “ShyLu20240321_.nwb” should be sufficient, and if not, can add “filter_name”: “Ripple 150-250 Hz”
Thanks @samuelbray32 !

@samuelbray32
Copy link
Collaborator Author

Sorry, can't test because I don't have write permission to /stelmo/nwb/analysis/ShyLu20240321/. Could you open that?

@xlsun79
Copy link
Contributor

xlsun79 commented Mar 29, 2024

@samuelbray32 thanks for testing!! Just opened the writing permission for ShyLu20240321 in the analysis folder.

@samuelbray32
Copy link
Collaborator Author

Got it. There was an outdated table ImportedLFPV1 in our database that was causing issue. drop_quick on that table solved:

dj.FreeTable(LFPOutput.connection,'`lfp_merge`.`l_f_p_output__imported_l_f_p_v1`').drop_quick()

Should be good to go now

@samuelbray32
Copy link
Collaborator Author

@CBroz1, is it possible this deprecated table re-entered the database because of a user on an outdated version of spyglass?

If so, is there a way to blacklist declaration of certain table names in the database to keep from happening again?

@CBroz1
Copy link
Member

CBroz1 commented Apr 1, 2024

Yes, it is possible to redeclare. I can imagine a simple and a complex solution.

The simple is to just declare a table with the same name, no fks, to cause a collision error on attempted declaration

The complex would involve intercepting declare on the mysql side, which seems like a maintenance burden if it's relatively few

Complex
-- Create list of table names we forbid
CREATE TABLE blacklist (
    table_name VARCHAR(100) PRIMARY KEY
);
-- Insert forbidden names
INSERT INTO blacklist (table_name) VALUES ('blacklisted_table1'), ('blacklisted_table2');
-- Add trigger to check against list on declare
DELIMITER $$

CREATE TRIGGER prevent_blacklisted_table
BEFORE CREATE ON DATABASE
FOR EACH STATEMENT
BEGIN
    DECLARE table_name VARCHAR(100);
    SET table_name = NEW.table_name;
    IF EXISTS (SELECT * FROM blacklist WHERE table_name = table_name) THEN
        SIGNAL SQLSTATE '45000' 
        SET MESSAGE_TEXT = 'Table name is blacklisted';
    END IF;
END$$
DELIMITER ;

@samuelbray32
Copy link
Collaborator Author

The simple solution makes sense to me. My guess is this will mainly show up as an issue between development commits so I don't know that the solution needs to scale between labs

@CBroz1
Copy link
Member

CBroz1 commented Apr 2, 2024

Reopening pending declaration of placeholder table

@CBroz1 CBroz1 reopened this Apr 2, 2024
@edeno
Copy link
Collaborator

edeno commented Apr 2, 2024

Do we know how common a problem we expect this to be? I am inclined to think we could deal with this on a case by case basis.

@xlsun79
Copy link
Contributor

xlsun79 commented Apr 2, 2024

Thank you @CBroz1 and @samuelbray32 and @edeno! I think I understand the solution conceptually, but am not sure what steps or actions I should take next. Would you be willing to take a crack at it and let me know if there's anything I could do to help resolve it?

@samuelbray32
Copy link
Collaborator Author

Sorry for confusion. It was temporarily working but someone must have loaded the old version again. It's functional now, will put in a more stable solution

@samuelbray32
Copy link
Collaborator Author

samuelbray32 commented Apr 2, 2024

@CBroz1, I don't know that the simple solution of declaring a dummy table will work in this case. I declared a dummy table in the database like this:

from spyglass.lfp import LFPOutput
import datajoint as dj
dj.FreeTable(LFPOutput.connection,'`lfp_merge`.`l_f_p_output__imported_l_f_p_v1`').drop_quick()
sql, external_stores = dj.declare.declare('`lfp_merge`.`l_f_p_output__imported_l_f_p_v1`', "dummy_id: varchar(8)", None)
sql = sql.format(database=LFPOutput.database)
LFPOutput.connection.query(sql)

Which successfully made the table. However, this dummy table now shows up in the parts of LFPOutput since dj gets parts based on name in the database (which the newly declared table does match) rather than relational dependency.

The other solution here is to ask people to updgrade spyglass (?)

@xlsun79 a temporary solution for you would be to add the line
dj.FreeTable(LFPOutput.connection,'`lfp_merge`.`l_f_p_output__imported_l_f_p_v1`').drop_quick()
before populating LFPBand in your code. This should ensure that the problem table isn't in the database when LFPOutput.fetch_nwb() is called during your population

@CBroz1
Copy link
Member

CBroz1 commented Apr 2, 2024

@samuelbray32 - That's a good point - I wasn't thinking about how the dummy table would show up as a part. I'm ok with the 'please upgrade' route - but we can go with the sql trigger route if we keep needing to drop

@samuelbray32
Copy link
Collaborator Author

Would the sql trigger have to intercept the table declaration? Or could it just raise when the table is declared and then delete?

This is new to me so no intuition on which is simpler to implement/run

@xlsun79
Copy link
Contributor

xlsun79 commented Apr 3, 2024

Thanks @samuelbray32 that worked for me =)

@CBroz1
Copy link
Member

CBroz1 commented May 7, 2024

I believe this has been fixed by facilitating an update for the user that was declaring this table. Please reopen if issues persist

@CBroz1 CBroz1 closed this as completed May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working merge To do with merge tables
Projects
None yet
Development

No branches or pull requests

4 participants