Python 3.9/3.11 support, dropping Python 3.6 support, and moving to GitHub Actions #559
Conversation
As of PyNWB 2.0.0, `SweepTable` is deprecated, although its use is still possible. We need to explicitly set `use_sweep_table=True` in `add_stimulus` and `add_acquisition` methods to build a sweep table. https://github.com/NeurodataWithoutBorders/pynwb/releases/tag/2.0.0
| @@ -236,36 +235,3 @@ def serialize(self, targets: Optional[OneOrMany[Dict[str, Any]]] = None): | |||
| file_.write(self._data.getvalue()) | |||
|
|
|||
| self._reload_nwbfile() | |||
|
|
|||
|
|
|||
| def set_container_sources( | |||
There was a problem hiding this comment.
I'm going to invoke Chesterton's Fence here and ask for an explanation of what this existing code does? Why is it safe to remove this now but not previously?
There was a problem hiding this comment.
I agree with the invocation.
But I've been looking through #361 and the commit making the changes (1aeb641) and I am yet to find a good source of documentation for why the method existed.
There are a couple of internal confluence pages that I'm trying to get access to in case they are enlightening.
If you have suggestions for where to look or who to check with, please let me know.
tests/test_nwb_utils.py
Outdated
| TEST_NWB_FILE_PATH = 'tests/data/nwb/Ctgf-T2A-dgCre;Ai14-495723.05.02.01.nwb' | ||
|
|
||
| def test_is_file_mies(): | ||
| assert is_file_mies(TEST_NWB_FILE_PATH) == True |
There was a problem hiding this comment.
Can you add a test to test for the other type? I think it was HBG?
There was a problem hiding this comment.
There is no specific test/check for HBG files. The usage is checking if the file is MIES, and if not it assumes it's HBG.
There is one nwb file in the repo that is not MIES, I could create a test that shows that the NWB data loads successfully when HBGNWBData class is used?
There was a problem hiding this comment.
I think we should have this requirements.txt be as flexible as possible in terms of dependency pinning. But we should have have our tests use a pinned-requirements.txt so that future maintainers don't get constantly hit with dependency update related test failures.
There was a problem hiding this comment.
Don't you want to know if the actual requirements.txt file customers are using will cause test failures either on install or on run? (likely because a dependency update caused an issue) I'm not following why it's a good idea to use different dependencies for tests vs. when pip installing.
After updating PyNWB, the pre-existing serialize method in the Nwb2Sink class was throwing errors along the form of: ValueError: Cannot change container_source once set: 'root' pynwb.file.NWBFile This commit resolves this error by removing the use of set_container_sources() After removal of the set_container_sources() method, tests pass and testing done by scientists also did not reveal any issues, but because the origin/reason for this code is not clear, we're not sure that removing it doesn't break something else with NWB files, for example with publishing to DANDI. Should you run into issues with containers or other metadata in NWB files, the work of the removed code might be the explanation. However it's not clear how to modify that code to get around the original error mentioned above.
Testing: re-created the images on the host for the self-hosted runner and re-ran the onprem tests.
sheriferson
force-pushed
the
feature/github-actions
branch
from
c814d79 to
9643dc1
Compare
Overview:
This PR does the following:
Type of Fix:
functionality to not work as expected)
Unit Tests:
See here for the light and onprem test results from my fork:
https://github.com/sheriferson/ipfx/actions/runs/6898536678
Checklist
Allen Institute Contribution Guidelines
Numpy Standards
appropriate
Notes
I believe this addresses #558, #557, and #550. With the move away from Python 3.6 and the unpinning of most dependencies,
pip install -e .succeeds on my M* Mac.Action items