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

Add requirements.txt #870

Merged
merged 5 commits into from
Feb 15, 2021
Merged

Conversation

ashgillman
Copy link
Member

Hopefully the Travis test will tell us if this is working - pip install -r requirements.txt seemed to be happy, but that was the extent of my testing.

The commented-out lines were not declared in the Wiki but were in Travis, so I expect they may be needed.

@ashgillman ashgillman requested a review from paskino February 11, 2021 11:09
.travis.yml Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@paskino
Copy link
Contributor

paskino commented Feb 11, 2021

This is what's installed in the VM, for comparison:

With pip: numpy scipy matplotlib nose coverage docopt deprecation nibabel pytest tqdm
With apt: python-dev python-pip python-tk python-pyqt5 python-pyqt5.qtsvg python-pyqt5.qtwebkit

@paskino
Copy link
Contributor

paskino commented Feb 11, 2021

As you suggested @ashgillman the requirements.txt is not available yet when issuing pip install -r requirements.txt because the repo hasn't been cloned yet.

Actually, I don't know well how we would be able to do it correctly. We clone the SuperBuild but requirements.txt is in SIRF which gets cloned only once we call make. We could fetch it from SIRF's repo, but we wouldn't necessarily be aware of which branch we are trying to build.

@KrisThielemans
Copy link
Member

yes it is. see https://travis-ci.org/github/SyneRBI/SIRF/jobs/758531042#L749

@ashgillman
Copy link
Member Author

From here, before merging, we need to add pip install -r optional-requirements.txt. I just want to make sure it runs without them first (i.e., that they are indeed optional)

@KrisThielemans
Copy link
Member

coverage and coveralls are used by ctest. However, our upload is currently broken. See #710 for a "solution" that didn't work.

requests is no (longer?) used. Potentially it is used in SIRF-exercises, but then it should be added there.

deprecation is currently not used anymore but was in the past and could be in the future. Please add it (always, not optional)

As opposed to creating an optional file, we could apparently use coveralls[test], see https://www.python.org/dev/peps/pep-0508/#extras

@ashgillman
Copy link
Member Author

Thanks for the info on those deps.

What you suggest is a little different. The notation coveralls[test] would be to install the "test" dependencies of "coveralls", which would have to be in turn specified in coveralls' setup.py - requirements.txt doesn't support extras (intentionally).

Reading through that, it is a bit more clear that requirements.txt is intended for users or deployment, as opposed to defining the dependencies of a project (which should be in setup.py).

So anything for CI etc. I guess should go in some form of requirements.txt (I would think maybe a ci-requirements.txt) and any dependencies could either go in a requirements.txt or we could put them into the setup.py.in.

requirements.txt Pros

  • easier for interested user to see in the source code - our setup.py.in is a little hard to find
  • closer aligned with what we wanted - just a way to communicate the dependencies to users and give them a simple way to install them.
  • Can install dependencies separate to installation

setup.py.in Pros

  • Can set up extras, like [test], [optional] (nibabel)
  • Dependencies are automatically installed during installation

Cons

  • Because the Python installation directory is non-standard, I'm not sure how well this works and where the dependencies go.

Because I can't see the project going up on the likes of PyPI any time soon, I don't think going the setup.py route is particularly necessary.

@KrisThielemans KrisThielemans linked an issue Feb 15, 2021 that may be closed by this pull request
@KrisThielemans KrisThielemans merged commit 2cd7275 into SyneRBI:master Feb 15, 2021
@ashgillman ashgillman deleted the add-requirements-txt branch February 15, 2021 12:20
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.

add requirements.txt for python prerequisites
3 participants