Remove bioread form required dependencies, ease extra modules installation

[smoia]

Closes #30

Proposed Changes

• bioread is no longer a mandatory package
• better handling of extra packages installation
• documentation for users and developers updated to reflect changes
• add bioread install in travis pipelines

[codecov]

Codecov Report

Merging #214 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #214   +/-   ##
=======================================
  Coverage   94.32%   94.32%           
=======================================
  Files           7        7           
  Lines         582      582           
=======================================
  Hits          549      549           
  Misses         33       33           

[RayStick]

@smoia I have submitted a PR to your branch, which contains a few minor wording and formatting corrections for the three rst docs that were edited.

For the other files, all the changes make sense. However, I did not test out any of these new installation options, as I have already got phys2bids installed. However, if you want me to test some, let me know. (I did run phys2bids locally and everything worked as expected).

[smoia]

Thank you @RayStick ! In fact, i cought a couple of typos more.
I merged the PR already, you can continue with the review process!

[RayStick]

All looks good.

As mentioned before, I did not specifically test out any of these new installation options, as I have already got phys2bids installed. However, if you want me to test some, let me know. (I did run phys2bids locally and everything worked as expected).

[smoia]

@RayStick if you think it's a good idea we can set up installation tests in Travis?

Otherwise, I tried by uninstalling phys2bids and reinstalling it a couple of times, but for me it's easier as I don't have conda (and installing python3 is easier for me).

[smoia]

@RayStick, apologies for the post-review changes.
I tried the installation a couple of times again and discovered that, in fact, it might fail due to the bioread installation.
Please have a look at the changes, and if you agree with them, we can ask a second reviewer for review!

[smoia]

Hi all! Can we try to merge this in? Can anyone else review this? @rmarkello @eurunuela or @vinferrer?

[eurunuela]

Sure, I’ll review it tomorrow. However, make sure the Travis test passes.

[RayStick]

@smoia Sorry for not getting back to you for a while. I won't have time to review the different installations options this week, so if @eurunuela is happy to review this tomorrow that'd be great.

[smoia]

@eurunuela unluckily Travis is failing due to reasons not related to the PR. I will try to restart it again, but if it keeps failing, please have a look at why it fails and what is the content of this PR, and possibly test it offline.

[eurunuela]

@smoia it looks like two of them fail due to a timeout.

However, test_acq.py and test_txt.py are failing for two reasons. Both have errors in downloading the test data, and test_txt.py seems to have some assertion errors. The issues with downloading the data look like there's been a timeout. This is what the error says:

urllib.error.ContentTooShortError: <urlopen error retrieval incomplete: got only 21979284 out of 21999903 bytes>

Do we know if there have been any issues with the OSF service lately?

I would not merge this until we can properly test the code. Assertion errors should not be happening and make me think that the code in this PR might be the reason for them.

I'll look at the code and see what lines could be making these fail.

[eurunuela]

These changes look good. I have a concern regarding the bioread version though.

[eurunuela]

Okay @smoia, I thought you were making more critical changes to the code. The tests should be passing for this PR. So, I say we merge this and open an issue to fix the tests.

I would not merge other PRs until the tests are fixed though.

We should probably add this issues with the tests to the agenda for our next meeting.

[smoia]

Let me change the minimal install requirement in Travis and we can merge!

[smoia]

@eurunuela

Ok, done. Should I merge it now or do we want to see if the CIs pass?

[eurunuela]

I think we can merge now. This PR should not alter the code in any way. I think it's safe.

[RayStick]

@smoia - I know I am down as Assignee but I will leave it to you to merge, as reading the discussion above I don't want to do it at the wrong time. :)