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

Fix make_or_load_nwbfile #1138

Closed
wants to merge 1 commit into from
Closed

Conversation

pauladkisson
Copy link
Member

@pauladkisson pauladkisson commented Nov 12, 2024

This PR fixes a bug with make_or_load_nwbfile that pops up whenever extensions are lazily imported.

Fixes #1114

See rly/ndx-pose#36 for detailed example.

@pauladkisson
Copy link
Member Author

pauladkisson commented Nov 12, 2024

@h-mayorquin, @rly,
This fix works*, but it rewrites any NWB files that are supposed to be appended, which could be slow for large NWB files. I couldn't come up with anything better but interested to know what you both think.

*works for my example -- failing for some reason for the test example...

@pauladkisson
Copy link
Member Author

Ok, based on @rly's comments, I think we should wait on this until we have a function in pynwb that can update an io with the current global namespace. Then, we can keep append-mode support and just update the existing io.

@pauladkisson pauladkisson deleted the fix_make_or_load_nwbfile branch November 13, 2024 19:59
@h-mayorquin
Copy link
Collaborator

@pauladkisson thanks for doing the detective work of tracking this bug and building an example. I tried this before and I failed.

This context manager has been a source of headaches and on top of that, I find it really difficult to read and understand. Cody and I wanted to break it into different pieces but never found the time and resources to do it.

If we could peel away the append mode functionality from the rest then we can create the io once all the calls to add_to_nwbfile are done and solve the problem for the 95 % of use cases. We can then fix the append mode once the necessary machinery on pynwb is added.

I think this is the way to move forward, let me know if there is something I am missing.

@rly
Copy link
Contributor

rly commented Nov 14, 2024

This context manager has been a source of headaches and on top of that, I find it really difficult to read and understand.

I agree. I know it is a Python feature, but I have never seen this syntax before and I find it unintuitive and confusing.

@pauladkisson
Copy link
Member Author

If we could peel away the append mode functionality from the rest then we can create the io once all the calls to add_to_nwbfile are done and solve the problem for the 95 % of use cases. We can then fix the append mode once the necessary machinery on pynwb is added.

Yeah, I think this is reasonable. What should we do with append in the mean time?

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Nov 14, 2024

We can discuss a couple of general solutions but, as I mentioned in #1114 I think the necessary imports for add_to_nwbfile to work should be called at the __init__ anyway. This will avoid this problem because all the uses of the context manager are inside run_conversion (which requires the interfaces to be already initialized and therefore the extension imports are already in place). Plus, I think it is good practice in general: Things should fail early if the extension library is missing in the executing environment.

With that said, I would still like to peel away the non-append mode functionality for two reasons:

  • Simplify this thing.
  • As a robust mechanism in case someone in the future forgets to do the __init__ imports.

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.

[Bug]: Issue with DLC interface not loading
3 participants