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

Instllation tips using uv #3503

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

samuelgarcia
Copy link
Member

No description provided.

Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

just edits and one comment before we go live with this.

installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
Steps:
1. On macOS and Linux. Open a terminal and do
`$ curl -LsSf https://astral.sh/uv/install.sh | sh`
1. On windows. Open a powershell and do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey Sam we recommend users use Command Prompt since our shell scripts for sorters only work in command prompt. It might be better if they have a command prompt version of instructions.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you test if the windows command also works in CMD ?
uv docs recommand powershell (I tested and it works)

installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
@zm711 zm711 added the packaging Related to packaging/style label Oct 24, 2024
Copy link
Collaborator

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

Hey @samuelgarcia this is very nice, I actually havn't tried uv yet so will run the commands to test on windows later. I request changes for some minor typos but feel free to ignore the other suggestions. BTW the creator of uv also makes another tool called Ruff 😏 maybe we should try it!

installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Show resolved Hide resolved

Choosing the installator + a environement manager + a package installer is a nightmare for beginners.
The main options are:
* use "anaconda" that do everything. The most popular but bad idea because : ultra slow and agressive licensing (not free anymore)
Copy link
Collaborator

@JoeZiminski JoeZiminski Oct 24, 2024

Choose a reason for hiding this comment

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

Maybe it is better to lead with the recommended one (i.e. uv first bullet point)?

On conda, is true in some cases but newer versions use mamba backend which is quite fast, and conda-forge channel does not have licensing issues. A problem is that HPC systems often use by default the older slow versions and the default channel is anaconda's.

No point going into too much detail but a suggestion to amend to say that that the default anaconda channel has agressive licensing and that it can be slow / is slower than uv. It is probably worth mentioning it can handle non-python dependencies while the other two can't, but this is not required for spikeinterface.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try to modify a bit

Copy link
Collaborator

Choose a reason for hiding this comment

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

conda+mamba via https://github.com/conda-forge/miniforge is my favorite, doesn't have licensing problems, and almost required when setting up gpu dependencies

Co-authored-by: Zach McKenzie <[email protected]>
Co-authored-by: Joe Ziminski <[email protected]>
@samuelgarcia
Copy link
Member Author

thanks a lot to @zm711 and @JoeZiminski.
At the moment the idea is to test th recipe.
We can reformulate later.


Kilosort, Ironclust and HDSort are MATLAB based and need to be installed from source.

### Quick installation

Steps:
1. On macOS and Linux. Open a terminal and do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two comments:

  1. I think it would be better to refer them to the uv installation guide. If it changes, the our docs will be outdated for no reason and they have os dependent paths:

https://docs.astral.sh/uv/getting-started/installation/

  1. Why are we creating new requirements? for the stabel if uv installs from pip it should suffice, for the rolling, uv can install directly from github and that would cover that.

In both cases it would use the pyproject.toml.

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 this is a good point for these external tools!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I do not want to overload the pyproject
The idea to have a working env for a beginner for a tutorial that contains spikeinterface but also external tools.
sigui sortingview ks.
The idea of having a entire and unqiue recipe and not refering to uv is to avoid a a benginer to understand evreything and compiling informations from several places.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand, you don't need to add anything to the pyproject. The point of the proposal is to have less files on the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here the requirements is bug : PySide6, jupyter.
I do not see this requirements as optional dependencies of spikeinterface but more a minimal stack for a beginner.
what would be the labels in pyproject.toml for theses 2 requirements files ?

Copy link
Collaborator

@h-mayorquin h-mayorquin Oct 25, 2024

Choose a reason for hiding this comment

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

Fair enough, the right way would be to use the newly accepted PEP 735 that covers just this but that is not available yet.

I would suggest that it would be better for you (so you don't have to remember about dependencies in two places) if the requirements files only had the dependencies that are not in the pyprojec.toml there. Then you could do:

uv pip install spikeinterface
uv pip install `the_beginnger_required_stack.txt` 

But maybe you don't mind these things being not reliable and updating them every school?

@samuelgarcia
Copy link
Member Author

BTW the creator of uv also makes another tool called Ruff 😏 maybe we should try it!

I know but I do not plan to use a linter very soon... I figth very hard to disable then in editors.

import spikeinterface.full as si
recording = si.load_extractor('./toy_example_recording')
sorting = si.run_sorter(sorter_name, recording, output_folder=f'./sorter_with_{sorter_name}', verbose=False)
sorting = si.run_sorter(sorter_name, recording, folder=f'./sorter_with_{sorter_name}', verbose=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we set n_jobs here? It might be a bit annoying during the install check that users see a million warnings about "n_jobs" not set.

Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

@samuelgarcia to get the env to work with the standard powershell and command prompt you can't use source + bin. So you need to fix these two commands.

installation_tips/README.md Outdated Show resolved Hide resolved
installation_tips/README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Related to packaging/style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants