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

Calls in the export process that don't trigger logging #1144

Closed
samuelbray32 opened this issue Oct 2, 2024 · 3 comments
Closed

Calls in the export process that don't trigger logging #1144

samuelbray32 opened this issue Oct 2, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request infrastructure Unix, MySQL, etc. settings/issues impacting users merge To do with merge tables

Comments

@samuelbray32
Copy link
Collaborator

Describe the bug
In testing the export and docker pipeline, found several cases where entries weren't logged for the SQL dump that we should probably include or make clear in documentation wont be hit. Listing here:

Using a table to restrict another

  • There were cases in my code where I select entries in table1 via table1 & (table2 & key) or table1 * (table2 & key)
  • Since i never fetch from table2 it doesn't get exported. This then changes behavior of the restriction in the docker
  • This could also be solved on the user end by ensuring call made as table1 & (table2 & key).fetch("KEY")

fetch_nwb

  • calling fetch_nwb from the merge table did not register the merge table entries in the export.
  • The entries of the parent source table were registered
  • Call in the code tracked for export: (LFPOutput & basic_key).fetch_nwb(restriction=basic_key)[0]["lfp"] where basic_key has restrictions for the source table LFPV1()
@samuelbray32 samuelbray32 added enhancement New feature or request merge To do with merge tables infrastructure Unix, MySQL, etc. settings/issues impacting users labels Oct 2, 2024
@samuelbray32
Copy link
Collaborator Author

fetch_nwb notes:

For the Merge.fetch_nwb() case:

  • Merge table entry: Not included
  • Parent Source entry: included
  • AnalysisNwbfile entry: not included
  • DandiPath: included

Tracing:

Missing Merge entry

  • Merge.fetch_nwb() doesn't do any logging before calling ParentSource.fetch_nwb(). explains why this not included
  • Maybe add in export logging within Merge.merge_restrict_class() to catch cases where the merge table is a "passthrough" for upstream tables

Missing AnalysisNwbfile entry

  • _log_fetch in the mixin ignores fetch calls from get_abs_path here
  • This means it's not logged when get_nwb_table calls get_abs_path here
  • fetch_nwb takes the filepath it gets from here and loads it with get_nwb_file. If this is an unopened local file, it accesses it with a pynwb io function that doesn't require any more fetch calls

It seems like the AnalysisNwbfile is maybe not getting hit as expected in the restriction graph from the SourceTable entry? (@CBroz1 )

@CBroz1
Copy link
Member

CBroz1 commented Oct 2, 2024

Thanks for putting this together @samuelbray32. I'll see what I can do about either capturing these missed cases or flagging them as items that the process would miss

@CBroz1 CBroz1 self-assigned this Oct 2, 2024
@CBroz1
Copy link
Member

CBroz1 commented Nov 20, 2024

Fixed in #1164

@CBroz1 CBroz1 closed this as completed Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request infrastructure Unix, MySQL, etc. settings/issues impacting users merge To do with merge tables
Projects
None yet
Development

No branches or pull requests

2 participants