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

Update installation Tips #1962

Merged
merged 15 commits into from
Sep 13, 2023
Merged

Update installation Tips #1962

merged 15 commits into from
Sep 13, 2023

Conversation

zm711
Copy link
Collaborator

@zm711 zm711 commented Sep 5, 2023

Last PR before I need to prep some school stuff.

I updated the installation tips since many users with issues have the si_env in the traceback they are using the ymls to create their envs. I moved to 3.10 for now (although I've been using 3.11 myself with no issues). I released the joblib and numpy restrictions and tested creating this env and running tridesclous with no issues. Bumped up tridesclous to the newest version and also bumped up neo to 0.12. Worked fine on windows, though I can test on a mac too later.

Also deleted elephant since it isn't a dependency. Is there a reason why we would want to keep having SI users download it to their env?

Finally, I added a tiny error log so that if someone runs the code and it fails it will at least say why things failed rather than be silent. So if a user has issues during testing they can at least share the error code on issues as semi-requested in #1870.

@zm711 zm711 marked this pull request as ready for review September 5, 2023 19:30
@zm711
Copy link
Collaborator Author

zm711 commented Sep 6, 2023

Tested on an M1 Mac now. Mountainsort caused the pip install to fail with this error:

ERROR: Failed building wheel for isosplit5
ERROR: Could not build wheels for isosplit5, which is required to install pyproject.toml-based projects

So I commented that out for now in the Mac install.
Running check on install led to the following:

OMP: Info #276: omp_set_nested routine deprecated, please use omp_set_max_active_levels instead.
write_binary_recording with n_jobs = 1 and chunk_size = 30000
write_binary_recording: 100%|##############################################################| 200/200 [00:02<00:00, 69.42it/s]
Import spikeinterface ...OK
Import spikeinterface.full ...OK
extract waveforms memmap: 100%|##############################################################| 77/77 [00:04<00:00, 17.26it/s]
Run tridesclous ...OK
Run herdingspikes ...Fail, Error: The sorter herdingspikes is not installed.Please install it with:  

To use HerdingSpikes run:

       >>> pip install herdingspikes
    More information on HerdingSpikes at:
      * https://github.com/mhhennig/hs2
     
Fitting PCA: 100%|█████████████████████████████████████████████████████████████████████████████| 9/9 [00:09<00:00,  1.02s/it]
Projecting waveforms: 100%|████████████████████████████████████████████████████████████████████| 9/9 [00:00<00:00, 77.34it/s]
Force compute_noise_levels() this is needed
qt.qpa.fonts: Populating font family aliases took 211 ms. Replace uses of missing font family "Menlo" with one that exists to avoid this cost. 
Open spikeinterface-gui ...OK
write_binary_recording: 100%|##############################################################| 200/200 [00:03<00:00, 59.66it/s]
Setting 'return_scaled' to False
extract amplitudes: 100%|################################################################| 200/200 [00:00<00:00, 5167.15it/s]
extract PCs: 100%|#########################################################################| 200/200 [00:03<00:00, 60.11it/s]
Export to phy ...OK

So I think adding in the exception actually provides pretty useful info for the user as can be seen by herding spikes failing in during the check install. Rather than do it silently it tells the user to pip install if they want it.

@alejoe91 alejoe91 added the installation Installation issues label Sep 6, 2023
@h-mayorquin
Copy link
Collaborator

@alejoe91 What do you think of adding test for this to the CI?

@zm711
Copy link
Collaborator Author

zm711 commented Sep 6, 2023

I think it is a good idea. Maybe just a chron job? This gets out of sync too often.

@alejoe91
Copy link
Member

alejoe91 commented Sep 6, 2023

Yep!! A chron job running on all three environments sounds great :)

@h-mayorquin
Copy link
Collaborator

Good. I will get to it.

@h-mayorquin
Copy link
Collaborator

Maybe we can merge this so we can test the latest changes? This looks good to me as it is.

@zm711
Copy link
Collaborator Author

zm711 commented Sep 6, 2023

I haven't tested linux, but both Mac (m1) and Windows (10) are confirmed to work with this code.

@zm711
Copy link
Collaborator Author

zm711 commented Sep 6, 2023

Actually the check_your_install.py opens the spikeinterface-gui. I've never tried doing Github Actions with gui-s. Will that work?

@h-mayorquin
Copy link
Collaborator

No, it won't work, that's a part that we will probably have to refactor and skip during CI.

@samuelgarcia
Copy link
Member

@alejoe91 What do you think of adding test for this to the CI?

@alejoe91 @h-mayorquin : Why not but this is very low priority, I think.
We have million of stuff more urgent on the tests side. No ?

I use this code when I prepare spike sorting workshop for students so we can quickly check theinstallation.

@zm711
Copy link
Collaborator Author

zm711 commented Sep 6, 2023

@alejoe91 @h-mayorquin : Why not but this is very low priority, I think.
We have million of stuff more urgent on the tests side. No ?

I use this code when I prepare spike sorting workshop for students so we can quickly check theinstallation.

I agree that there are more urgent tests in general, but I've linked a few of the issues where people use the install tips as their way of creating a spikeinterface environment. Making sure that beginners first experience with spikeinterface is a good one (with their local 'test' passing) is a good way to keep them using the package.

Open: #1915, #1870, #1476, #1447,
Closed: #1252, #1253, #1200, #1194

@zm711
Copy link
Collaborator Author

zm711 commented Sep 7, 2023

I updated the linux yaml to release the version restrictions, but can't test locally to make sure it works.

I finally hacked together a workflow that checks to make sure that the conda environment can be created on a cron job. That way it'll be easier to add tests later, but at least for now we can see if the environment gets out of sync sooner rather than with a user issue. You want to take a peek at the yml @h-mayorquin (I always mess them up as you can see with the commit history)? It is pretty simple for now. That way a refactor/proper test can be pushed off to a later time for the check_your_install.py.

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Sep 7, 2023

@zm711
Thanks for collecting all of those issues where the problem of the environment appears. This is very useful for reference.

I also think that having new users face as little difficulties as possible is very important, it will also make our life easier in the long-term when we switch from developing futures to support. In this case, the script that you wrote which should test that the conda environment is working correctly is a step in the right direction and hopefully -as you did it- will alleviate @samuelgarcia concerns about priorities.

Long term though I think we should do two things:

  1. Run the full testing suite in the matrix of the all the operating systems for the cron test.
  2. Reference the conda environment to the pip installation so we don't scatter around the specification of our dependencies (harder to track). That way, we can instruct users of other operating systems that are facing a bug to just use our testing suite.

@alejoe91
Copy link
Member

Thanks so much @zm711

I'll have a final round with @samuelgarcia on Wednesday to merge some PRs including this one! Thanks for the patience!

@zm711
Copy link
Collaborator Author

zm711 commented Sep 11, 2023

No worries @alejoe91! I just didn't want to let things get out of sync, but I'll let you merge main into it whenever you are ready to merge it.

@alejoe91 alejoe91 merged commit 5ee4563 into SpikeInterface:main Sep 13, 2023
8 checks passed
@zm711 zm711 deleted the check_install branch September 13, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation Installation issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants