Skip to content

changing the traits minimum version #1791

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

Merged
merged 6 commits into from
Feb 22, 2017
Merged

changing the traits minimum version #1791

merged 6 commits into from
Feb 22, 2017

Conversation

djarecka
Copy link
Collaborator

The new traits version probably solves problems from #1788.

@satra
Copy link
Member

satra commented Jan 30, 2017

@oesteban - do you recollect why our docker builds don't use miniconda?

@djarecka
Copy link
Collaborator Author

Ok, so Python 2.7 from my OSX and from the nipype_test container with traits 4.6 worked very well, but it looks like py27 from Travis works even worse than before...

@satra
Copy link
Member

satra commented Jan 30, 2017

@djarecka - perhaps i can put this on your plate - we had long considered switching from traits to traitlets (by the ipython folks) - perhaps we can start looking into that again. right now the ci failure has to do with building traits since it's not using conda. but the py27 failure is strange and also why it happens only in one branch of the test.

@djarecka
Copy link
Collaborator Author

@satra - CircleCI has indeed problem with installation (I manually installed with conda for testing), but Travis has segfault.

What do you mean by writing that it happens in one branch of the test?

@djarecka
Copy link
Collaborator Author

@satra - I can read about traitlets, never heard before

@codecov-io
Copy link

codecov-io commented Jan 30, 2017

Codecov Report

Merging #1791 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1791      +/-   ##
==========================================
- Coverage   72.71%   72.71%   -0.01%     
==========================================
  Files        1059     1059              
  Lines       52550    52553       +3     
==========================================
+ Hits        38212    38214       +2     
- Misses      14338    14339       +1
Flag Coverage Δ
#unittests 72.71% <100%> (-0.01%)
Impacted Files Coverage Δ
nipype/info.py 84.61% <100%> (ø)
nipype/pipeline/engine/tests/test_utils.py 84.48% <100%> (+0.19%)
nipype/interfaces/afni/base.py 65.59% <ø> (-1.08%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20f0444...621cc61. Read the comment docs.

@oesteban
Copy link
Contributor

oesteban commented Feb 1, 2017

@satra we used miniconda to get thinner images, but we could perfectly jump into anaconda, it is much more reliable.

Right now we don't rely that much on traits (nothing traitlets couldn't do). If we don't intend to make a larger use of traits (their interfacing abilities, linking traits, etc), we are probably better off moving into traitlets.

@oesteban
Copy link
Contributor

@djarecka, hope this helps:

@djarecka
Copy link
Collaborator Author

@oesteban - thanks for the suggestions. In the last commit I simply removed my changes from requirements.txt since was trying to understand the new import error (#1798), but I will update requirements.txt. Just wanted to wait for your PR (#1796) to be accepted.

I understood that @satra asked about miniconda in docker, since I was getting an error when I was trying to import new traits (see https://circleci.com/gh/djarecka/nipype/138). At the end I'm not sure if we're planning to move to miniconda for docker builds.

@oesteban
Copy link
Contributor

Got you.

Let's start with the second bit: your build is failing because the docker image does not have gcc installed to compile stuff. Best alternative is use conda to install it (not pip) since it will install only binaries.

All about docker images needs a cleanup, I'll try to submit a PR later today or tomorrow. I will remove all those unnecessary docker files and update the necessary ones to be more lightweight and clearer.

Regarding the icu installation, we merged #1808 on Friday. That should get you going without the import error.

My PR #1796 should not affect you much (however, I've made some fixes on the travis file that you probably want to have asap).

To summarize this PR:

  • You need an update of the docker images (so that you can add traits in the appropriate conda install)
  • You need tst: not defining specific icu #1808 merged into this branch.
  • Then there is this two-fold dependency management issue: 1) updating the info.py file (for the pypi distribution) and 2) updating the requirements.txt (for the development environments). Since we are using conda in travis and circle, we need to add travis>=4.6 manually (until the conda-forge installation of nipype sets that dependency). Maybe @chrisfilo could chime in and give us some lines about how to pin package versions for nipype in the conda-forge channel. I guess we need to update nipype there as well.

@satra
Copy link
Member

satra commented Feb 13, 2017

@oesteban - if we can make these docker updates, we can update nipype on conda-forge. we should definitely add icu as a dependency for the lxml library if it's not already there.

@satra
Copy link
Member

satra commented Feb 13, 2017

@oesteban - we should release 0.13 soon - it's been sitting in release candidate mode, but should really merge in these latest fixes and as long as the builds pass we release.

@djarecka
Copy link
Collaborator Author

@oesteban - great, I'll wait for a new docker files.

I also have general question about the docker images. Please correct me if I'm wrong, but we're building docker images in CircleCI based on the current version of docker files. However, if we ask people to test "locally" they have to pull images from docker hub, and this was not updated for the last 5 months https://hub.docker.com/r/nipype/nipype_test/~/dockerfile/. So currently I have to always install pytest (and few other libraries) if I really want to use those images for testing. Should we also think about docker hub versions or I'm the only one using those images?

@oesteban
Copy link
Contributor

Good point. @chrisfilo has done a great investigation on that. The idea would be pushing (automatically, from CircleCI) a new image when a release is done.

@oesteban
Copy link
Contributor

For developers, I'd say they should build their own docker image from the dockerfile. The built image should be up-to-date if all nipype dependencies are well-captured (meaning, no need to install pytest for instance).

@djarecka
Copy link
Collaborator Author

having various problem with CircleCI build: https://circleci.com/gh/djarecka/nipype/147 or https://circleci.com/gh/djarecka/nipype/146.
@oesteban - do you have any suggestions?

@djarecka
Copy link
Collaborator Author

@satra - I merged with master, so Circle CI works. This PR does not solve the segfault problem IMO, but the segfault might happen less often.

@oesteban
Copy link
Contributor

LGTM, after merging this, we can close #1809

@satra satra merged commit 5744d60 into nipy:master Feb 22, 2017
def test_traits_version():
#just a temprorary test, testing CI
import traits
assert traits.__version__ >= "4.6.0"
Copy link
Member

Choose a reason for hiding this comment

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

merged this a little too quickly, while this works most of the time, we should use LooseVersion, which does a better check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Actually, we should have like a pre-install hook (we've talked about it) to check versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually that was my temporary test, when wanted to be sure that Travis indeed uses traits4.6 and still gives the segfault.
I should probably just remove this test...

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.

4 participants