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

close analysis nwb files #1193

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

close analysis nwb files #1193

wants to merge 4 commits into from

Conversation

jguides
Copy link
Collaborator

@jguides jguides commented Nov 22, 2024

This pull request has changes to avoid the error:
OSError: [Errno 24] Too many open files: '/home/jguidera/Src/jguides_2024/src/jguides_2024'

This error results from analysis nwb files being left open after being fetched.

@samuelbray32
Copy link
Collaborator

Leaving the files open by default gives performance improvements when reaccessing data in a python process

If you hit this error, you can run this to clear up you open files instead:

from spyglass.utils.nwb_helper_fn import close_nwb_files
close_nwb_files()

@@ -96,6 +96,11 @@ def get_nwb_file(nwb_file_path):
nwbfile = io.read()
__open_nwb_files[nwb_file_path] = (io, nwbfile)

__open_nwb_files.clear()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this clear the cache every time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I agree this doesn't seem ideal and have removed the line.

@@ -294,7 +296,7 @@ def fetch_nwb(query_expression, nwb_master, *attrs, **kwargs):
get_nwb_file(file_path)

query_table = query_expression.join(
tbl.proj(nwb2load_filepath=attr_name), log_export=False
Copy link
Collaborator

Choose a reason for hiding this comment

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

#1192 Has a fix for the log_export for non-SpyglassMixin tables that will still let the export work. Would prefer waiting for that to merge than pushing this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you revert the removals of log_export?

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 current failure of tests is because its not being logged

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done! Hopefully this works.

@@ -237,7 +237,7 @@ def get_nwb_table(query_expression, tbl, attr_name, *attrs, **kwargs):
# TODO: check that the query_expression restricts tbl - CBroz
nwb_files = (
query_expression.join(
tbl.proj(nwb2load_filepath=attr_name), log_export=False
Copy link
Collaborator

Choose a reason for hiding this comment

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

see comment refering #1192 below

@jguides jguides requested a review from samuelbray32 November 26, 2024 16:22
@edeno
Copy link
Collaborator

edeno commented Nov 26, 2024

Hi @jguides, do you happen to have a snippet of code that creates the problem so we can evaluate the solution?

@jguides
Copy link
Collaborator Author

jguides commented Nov 26, 2024

Hi @jguides, do you happen to have a snippet of code that creates the problem so we can evaluate the solution?

Hi @edeno, this was happening with population of some custom tables that rely on multiple data entries. I think this would recreate it:

from src.jguides_2024.firing_rate_vector.jguidera_well_event_firing_rate_vector import TimeRelWAFRVecSTAveSummSel
PathFRVecSTAveSummSel().insert_defaults()

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.

3 participants