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

Remove conda env file and improve README #753

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Conversation

CodyCBakerPhD
Copy link
Member

@CodyCBakerPhD CodyCBakerPhD commented Feb 19, 2024

Closes #743
Fixes #742

@CodyCBakerPhD CodyCBakerPhD self-assigned this Feb 19, 2024
Copy link
Member

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

Do you think we should mention here which versions we support? That way a new user wouldn't have to guess which version to select for their conda environment.

@CodyCBakerPhD
Copy link
Member Author

Do you think we should mention here which versions we support?

If you can think of a way to do that in text without needing to update it over time (which we're inevitably bound to forget if its decentralized) I'm all for it

Otherwise, the attempt to pip install neuroconv (without specific version) will always route them to the latest/last release for each Python version they might realistically select (or warn that it is not available otherwise)

```shell
pip install neuroconv
```

For more flexibility we recommend installing the latest version directly from GitHub. The following commands create an environment with all the required dependencies and the latest updates:
To install the current unreleased `main` branch (requires `git` to be installed in your environment, such was via `conda install git`), run:
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 it would be better to point them to the official instalation instructions of the git project:
https://git-scm.com/downloads

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK, that tends to do different things than you would expect depending on the platform. Suggested added both just in case

On windows for instance, it's a standalone interpreter with it's own console (doesn't play well with standard command prompt)

You can technically add it to the system registry but behaves best with conda when installed as a separate package

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see. Ok, I did not see this before writing my comment below. Ok, so in your experience installing conda with git is the easier for windows users and hence you are making that suggestion.

The current two-fold solution seems fine but we could also explicitly mention that we suggest git on windows. As you feel is better.

README.md Outdated
pip install git+https://github.com/catalystneuro/neuroconv.git@main
```

To install the package in [editable mode](https://pip.pypa.io/en/stable/cli/pip_install/#editable-installs), run:
Copy link
Collaborator

@h-mayorquin h-mayorquin Feb 20, 2024

Choose a reason for hiding this comment

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

Just for your information.

For another PR as you are just refactoring but I think we probably should remove mentions of editable installs on general readme documents and send them over to the developer sections.

With the migration away from setup tools as a default, editable installs are less and less consistent. I jumped into this rabbit hole last week. Just two point out two common problems:

  • Poetry which is one of the most pouplar npm-like experiences for python does not suppport editable installs.
  • static analysis tools would not be able to use the enabled editable installs because they use import hooks:

microsoft/pylance-release#3473

I can take care of this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed editable install instructions; I believe the idea of this README duplicating our user guide in the docs is to reduce redirection for people just finding the package on PyPI or main GitHub page. So just quickest and dirtiest instructions to get going. Otherwise I'm in greater favor of going the GUIDE README approach and offloading everything of detail to the main docs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good. Not sure if they should be removed from the cookie cutter for the conversion projects as well. Here we expect users, there we expect people to change stuff? leaning to remove it as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Conversion projects depend on the client. Some want/need to tamper with stuff, so yes to editable there

Others want static PyPI release with CLI tooling; what we do most with them with respect to bug fixes is do the git based pip install of a particular branch or commit

@h-mayorquin
Copy link
Collaborator

This is OK to me. If no one else is using these files I will trust @CodyCBakerPhD gut that the mainentance is not worth the convenience.

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) February 20, 2024 21:53
@pauladkisson
Copy link
Member

Do you think we should mention here which versions we support?

If you can think of a way to do that in text without needing to update it over time (which we're inevitably bound to forget if its decentralized) I'm all for it

Some people (ex. python-gitlab) look like they have pypi keep track of it and then link a badge, but tbh it seems like a bit more work than its worth in this case.

@CodyCBakerPhD CodyCBakerPhD merged commit 22318d9 into main Feb 21, 2024
21 of 22 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the remove_conda_env_file branch February 21, 2024 01:17
@CodyCBakerPhD
Copy link
Member Author

Some people (ex. python-gitlab) look like they have pypi keep track of it and then link a badge, but tbh it seems like a bit more work than its worth in this case.

Oooo that is a nice looking actually

@CodyCBakerPhD CodyCBakerPhD mentioned this pull request Feb 21, 2024
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]: make_environment.yml leads to installation error
3 participants