-
Notifications
You must be signed in to change notification settings - Fork 191
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
Add plexon2 recording, sorting and event support #1736
Conversation
It looks like not all required packages are installed for the CI tests. As far as I can see you are installing the latest version of the neo package, but not |
|
||
mode = "file" | ||
NeoRawIOClass = "Plexon2RawIO" | ||
handle_spike_frame_directly = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you have a look to #1626 @h-mayorquin made some variable name changes.
Thanks a lot @JuliaSprenger |
@alejoe91 do we include this also in the relase no ? |
Is it released on neo? |
@JuliaSprenger tests seem to fail because of |
@samuelgarcia @h-mayorquin good to merge for me! I included the missing dependencies in CI and updated after #1626 |
- name: Install wine (needed for Plexon2) | ||
run: | | ||
sudo rm -f /etc/apt/sources.list.d/microsoft-prod.list | ||
sudo dpkg --add-architecture i386 | ||
sudo apt-get update -qq | ||
sudo apt-get install -yqq --allow-downgrades libc6:i386 libgcc-s1:i386 libstdc++6:i386 wine | ||
shell: bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be quite heavy and not needed (unless plexon2 wrapper changes). We could make a manual workflow that installs wine and tests plexon2 only.
Otherwise we can make an install_wine.yml
action and install + run plexon2 tests only if plexon2 has changed.
@samuelgarcia done with the suggestions! |
@@ -290,6 +290,32 @@ def test_pickling(self): | |||
pass | |||
|
|||
|
|||
# We mark plexon2 tests as they require additional dependencies (wine) | |||
@pytest.mark.plexon2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In POSIX the dependencies are wine and https://pypi.org/project/zugbruecke/ right?
In windows what are the dependecies?
I don't like that if I don't have wine or this experimental packages my test will fail if run pytest -m "extractors" locally.
Can we add skipif tests to accomplish this? (this can be another PR if you think this is too much)
I am suggesting:
import subprocess
import platform
def has_dependencies():
"""
Check if required dependencies are installed.
"""
os_type = platform.system()
if os_type == "Windows":
# On Windows, check for dependencies if any (probably just return True)
return True
elif os_type == "Linux":
# Check for 'wine' using dpkg
try:
result_wine = subprocess.run(['dpkg', '-l', 'wine'],
stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True)
except subprocess.CalledProcessError:
return False
# Check for 'zugbruecke' using pip
try:
import zugbruecke
return True
except subprocess.CalledProcessError:
return False
else:
raise ValueError(f"Unsupported OS: {os_type}")
# Use pytest.mark.skipif to conditionally skip the test class
@pytest.mark.skipif(not has_dependencies(), reason="Required dependencies not installed")
@pytest.mark.plexon2
class Plexon2RecordingTest(RecordingCommonTestSuite, unittest.TestCase):
ExtractorClass = Plexon2RecordingExtractor
downloads = ["plexon"]
entities = [
("plexon/4chDemoPL2.pl2", {"stream_id": "3"}),
]
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that maybe this will help us avoid adding -some- complexity to the testing in the CI but less sure about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @h-mayorquin , I'll do something along these lines!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. One last question about the CI logic.
.github/workflows/full-test.yml
Outdated
- name: Test core | ||
run: ./.github/run_tests.sh core | ||
- name: Test extractors | ||
if: ${{ steps.modules-changed.outputs.EXTRACTORS_CHANGED == 'true' || steps.modules-changed.outputs.CORE_CHANGED == 'true' }} | ||
run: ./.github/run_tests.sh "extractors and not streaming_extractors" | ||
run: ./.github/run_tests.sh "extractors and not streaming_extractors and not plexon2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need not here with the new condition that you added? Will it not be skipped automatically when wine is not installed and test it with all the other extractor when wine is installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, removed!
.github/workflows/full-test.yml
Outdated
@@ -157,3 +164,6 @@ jobs: | |||
- name: Test internal sorters | |||
if: ${{ steps.modules-changed.outputs.SORTERS_INTERNAL_CHANGED == 'true' || steps.modules-changed.outputs.SORTINGCOMPONENTS_CHANGED || steps.modules-changed.outputs.CORE_CHANGED == 'true' }} | |||
run: ./.github/run_tests.sh sorters_internal | |||
- name: Test plexon2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way, we would not need this, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
This PR is building on top of NeuralEnsemble/python-neo#1214 and can only be merged once the pl2 format is supported by Neo.