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 dipy recipe. #308

Merged
merged 11 commits into from
May 18, 2016
Merged

Add dipy recipe. #308

merged 11 commits into from
May 18, 2016

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Apr 10, 2016

No description provided.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dipy) and found it was in an excellent condition.

@arokem
Copy link
Contributor Author

arokem commented Apr 10, 2016

Replaces #307

requirements:
build:
- python
- numpy
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that, since this requires numpy and cython, it uses the numpy ABI. If That is true you need to use: numpy x.x in the build and run fields.

@arokem
Copy link
Contributor Author

arokem commented Apr 10, 2016

This one depends on #284

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dipy) and found it was in an excellent condition.

@arokem
Copy link
Contributor Author

arokem commented May 10, 2016

@jakirkham
Copy link
Member

We intentionally downgrade conda-build. See this PR ( #488 ) and related cross refs for details.

@arokem
Copy link
Contributor Author

arokem commented May 10, 2016

I'm letting this run, but I should probably reduce testing here.

@arokem
Copy link
Contributor Author

arokem commented May 12, 2016

Does anyone have any thoughts on why we are getting this conflict?

requirements:
build:
- python
- numpy >=1.7.1
Copy link
Member

Choose a reason for hiding this comment

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

You don't really need this. We build only numpy 1.9 and above.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I check setup.py and I believe you need numpy x.x to ensure the numpy used in the build will be installed at run time.

@arokem
Copy link
Contributor Author

arokem commented May 12, 2016

Like so?

- numpy x.x
- scipy
- nibabel
- cython
Copy link
Member

Choose a reason for hiding this comment

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

Do you need cython as a run dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Probably not.

On Thu, May 12, 2016 at 7:14 AM, Filipe notifications@github.com wrote:

In recipes/dipy/meta.yaml
#308 (comment)
:

+build:

  • number: 0
  • script: python setup.py install

+requirements:

  • build:
    • python
    • numpy x.x
    • nibabel
    • cython
  • run:
    • python
    • numpy x.x
    • scipy
    • nibabel
    • cython

Do you need cython as a run dependency?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/conda-forge/staged-recipes/pull/308/files/11806b214f840b1bb21af55204299c8d7a715980#r63027341

@grlee77
Copy link
Member

grlee77 commented May 12, 2016

IIRC dipy also compiles using OpenMP if available. To enable it (at least for osx and linux),you can add
- gcc # [unix] to the build requirements
- libgcc # [unix] to the run requirements

@ocefpaf
Copy link
Member

ocefpaf commented May 12, 2016

@grlee77 I prefer the more honest # [not win] 😄

@grlee77
Copy link
Member

grlee77 commented May 12, 2016

Also, I am not an export on the windows builds, but I think that will require something like:

features:
   - vc9                                 # [win and py27]
   - vc10                                # [win and py34]
   - vc14                                # [win and py35]

to appear within the build section. (you can see an example in #453)

@grlee77
Copy link
Member

grlee77 commented May 12, 2016

The CircleCI build failures are because the VTK package for conda-forge hasn't yet been merged and the default channel only has a Python 2.7 one. So Python 3.4 and 3.5 won't build until #453 is completed.

@grlee77
Copy link
Member

grlee77 commented May 12, 2016

Alternatively, if you really want this merged asap, you could remove VTK from the runtime dependencies (I believe it is optional). It could be added back in the feedstocks after vtk is merged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/dipy) and found some lint.

Here's what I've got...

For recipes/dipy:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form.

@arokem
Copy link
Contributor Author

arokem commented May 12, 2016

Ah yes, VTK on Python 3. Thanks @grlee77 for the pointer (and thanks for the work on #453. Let's get this in without VTK for now, and add it later on once that's settled.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dipy) and found it was in an excellent condition.

@arokem
Copy link
Contributor Author

arokem commented May 12, 2016

Is there a way to say something like "add VTK to run, but only on Python 3"?

Might this be the way to achieve that?

- vtk [py34 or py35]

@ocefpaf
Copy link
Member

ocefpaf commented May 12, 2016

@arokem it can be vtk # [py3k]

@arokem
Copy link
Contributor Author

arokem commented May 12, 2016

Sorry - got myself confused there. Need VTK, but only on python 2. Maybe this will do it.

@@ -32,6 +32,7 @@ requirements:
- scikit-learn
- matplotlib
- libgcc # [not win]
- vtk # [py27]
Copy link
Member

Choose a reason for hiding this comment

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

👍

@grlee77
Copy link
Member

grlee77 commented May 12, 2016

The Travis CI is a time-out, but what is odd is that if you grep for TEST END: dipy, it does seems to have completed all 6 dipy builds (numpy 1.10, 1.11) * (python 2.7, 3.4, 3.5).

What I don't understand is why it then tries to build glueviz? see the Building glueviz-0.7.3-py27_0 near the end of the log

@grlee77
Copy link
Member

grlee77 commented May 14, 2016

I think Travis should pass now if restarted.

@jakirkham
Copy link
Member

jakirkham commented May 14, 2016

The Travis CI is a time-out, but what is odd is that if you grep for TEST END: dipy, it does seems to have completed all 6 dipy builds (numpy 1.10, 1.11) * (python 2.7, 3.4, 3.5).

There seems to be an issue generating feedstocks ( #619 ). As a result, there are a bunch recipes still included in staged-recipes that need to be converted. Once that is resolved, we would be more than happy to restart and expect this will pass.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dipy, recipes/lua-5.1, recipes/pycapnp) and found it was in an excellent condition.

@arokem
Copy link
Contributor Author

arokem commented May 15, 2016

OK. For now, I have rebased this on master, which included these other recipes in, and I will rebase again once that's resolved.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dipy) and found it was in an excellent condition.

@arokem
Copy link
Contributor Author

arokem commented May 18, 2016

Rebased again. I believe this should be ready to go now?

@ocefpaf
Copy link
Member

ocefpaf commented May 18, 2016

LGTM

@ocefpaf ocefpaf merged commit ebf6066 into conda-forge:master May 18, 2016
extra:
recipe-maintainers:
- arokem
- garyfallidis
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure you guys have already discussed this, but please let us know whether it is ok to have you as a maintainer on the dipy recipe at conda-forge, @Garyfallidis.

@Garyfallidis
Copy link

All good here. But please replace nipy.org/dipy to dipy.org if possible.

@grlee77
Copy link
Member

grlee77 commented May 18, 2016

@arokem, @Garyfallidis
you may already be aware of this, but thought I would mention it as I forgot the forking step my first time around...

Since this has already been merged the procedure to change the recipe now is to fork the feedstock from https://github.com/conda-forge/dipy-feedstock, create a branch with the desired change and then submit a PR there. (If you are just changing the website link, I think you can skip Travis and Appveyor by adding [skip ci] at the end of the commit message)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants