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

Convert to py3 #8

Closed
wants to merge 5 commits into from
Closed

Convert to py3 #8

wants to merge 5 commits into from

Conversation

sanjayankur31
Copy link

First bits for #7

@sanjayankur31 sanjayankur31 changed the title Convert to py3 using 2to3 Convert to py3 Jan 14, 2019
@sanjayankur31
Copy link
Author

I've manually modified the rest, and tweaked errors. Now, there's only the one test that fails with py3 that I can'f figure out at the moment.

@unidesigner
Copy link
Contributor

Ok, thanks. Which test?

@sanjayankur31
Copy link
Author

It's cfflib.tests.test_cfflib.test_save_load. The complete output is here:

$ python setup.py build && python setup.py install && nosetests
running build
running build_py
copying cfflib/tests/test_cfflib.py -> build/lib/cfflib/tests
copying cfflib/tests/data/CNetwork/network_res83.graphml -> build/lib/cfflib/tests/data/CNetwork
copying cfflib/tests/data/CSurface/labels.gii -> build/lib/cfflib/tests/data/CSurface
running install
running build
running build_py
running install_lib
copying build/lib/cfflib/tests/test_cfflib.py -> /home/asinha/dump/cfflib-virt/lib/python3.7/site-packages/cfflib/tests
copying build/lib/cfflib/tests/data/CSurface/labels.gii -> /home/asinha/dump/cfflib-virt/lib/python3.7/site-packages/cfflib/tests/data/CSurface
copying build/lib/cfflib/tests/data/CNetwork/network_res83.graphml -> /home/asinha/dump/cfflib-virt/lib/python3.7/site-packages/cfflib/tests/data/CNetwork
byte-compiling /home/asinha/dump/cfflib-virt/lib/python3.7/site-packages/cfflib/tests/test_cfflib.py to test_cfflib.cpython-37.pyc
running install_egg_info
Removing /home/asinha/dump/cfflib-virt/lib/python3.7/site-packages/cfflib-2.0.5-py3.7.egg-info
Writing /home/asinha/dump/cfflib-virt/lib/python3.7/site-packages/cfflib-2.0.5-py3.7.egg-info
....E......
======================================================================
ERROR: cfflib.tests.test_cfflib.test_save_load
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/asinha/dump/cfflib-virt/lib64/python3.7/site-packages/nose/case.py", line 134, in run
    self.runTest(result)
  File "/home/asinha/dump/cfflib-virt/lib64/python3.7/site-packages/nose/case.py", line 152, in runTest
    test(result)
  File "/usr/lib64/python3.7/unittest/case.py", line 680, in __call__
    return self.run(*args, **kwds)
  File "/usr/lib64/python3.7/unittest/case.py", line 640, in run
    self._feedErrorsToResult(result, outcome.errors)
  File "/usr/lib64/python3.7/unittest/case.py", line 570, in _feedErrorsToResult
    result.addError(test, exc_info)
  File "/home/asinha/dump/cfflib-virt/lib64/python3.7/site-packages/nose/proxy.py", line 132, in addError
    self.result.addError(self.test, self._prepareErr(err))
  File "/home/asinha/dump/cfflib-virt/lib64/python3.7/site-packages/nose/result.py", line 61, in addError
    exc_info = self._exc_info_to_string(err, test)
  File "/home/asinha/dump/cfflib-virt/lib64/python3.7/site-packages/nose/result.py", line 187, in _exc_info_to_string
    return _TextTestResult._exc_info_to_string(self, err, test)
  File "/usr/lib64/python3.7/unittest/result.py", line 186, in _exc_info_to_string
    exctype, value, tb, limit=length, capture_locals=self.tb_locals)
  File "/usr/lib64/python3.7/traceback.py", line 515, in __init__
    self.filename = exc_value.filename
AttributeError: 'ParseError' object has no attribute 'filename'

----------------------------------------------------------------------
Ran 11 tests in 0.689s

FAILED (errors=1)

@sanjayankur31
Copy link
Author

@unidesigner : any ideas on this?

@liadomide : do you think someone from the TVB team that knows about cfflib could take a look and complete this PR please? I'm working on tvb-library for NeuroFedora now, and as you'd confirmed cfflib is a necessary dependency there.

Cheers!

@unidesigner
Copy link
Contributor

@liadomide Are you really using cfflib?

@liadomide
Copy link

Yes, we are reading CFF files, but in tvb-framework.
If this becomes too difficult to maintain, we could "adopt" in tvb codebase only the parts we need for reading CFF files.
https://github.com/the-virtual-brain/tvb-framework/blob/master/tvb/adapters/uploaders/cff_importer.py

@sanjayankur31
Copy link
Author

That would be good, @liadomide : no one else has come forward as a cfflib user (yet), so there is a chance that TVB may be the only tool that uses it.

@unidesigner : what do you think?

@unidesigner
Copy link
Contributor

Indeed I think TVB is the only tool (except connectomeviewer, but that's also not maintained anymore) who I think has used it in the past. The reason was probably that I released some datasets as cfflib files which were interesting to import into TVB for analyses, but that was it. cfflib is basically a container format, and there are better efforts now, such as BIDS http://bids.neuroimaging.io/ - and individual contained file formats have also probably evolved quite a bit individually.
So my suggestion would be to remove cff_importer.py from TVB and get rid of the dependency althogether and not include it in NeuroStars.

@sanjayankur31
Copy link
Author

Not using cfflib sounds good as a long term solution if cfflib is not going to be maintained now, but there seems to be community that is using it now. So we really can't suddenly pull the plug on it---they'll have to be given time to migrate to BIDS etc. So, could cfflib be patched up to be usable in the meantime?

@unidesigner
Copy link
Contributor

Sorry, I think I misread your comment and I thought that no one has come forward as cfflib user from TVB.
Can @liadomide give a definitive answer?

@sanjayankur31
Copy link
Author

Sorry, I think I misread your comment and I thought that no one has come forward as cfflib user from TVB.

Ah, no---there are quite a few TVB users that rely on cfflib and therefore, they cannot drop it from TVB.

Can @liadomide give a definitive answer?

I discussed this with @liadomide over e-mail before when I began working on TVB. I had e-mailed to ask if I could simply drop the cfflib bits since it didn't work with Python 3. She confirmed that cfflib is a necessary component of TVB, and that is why I tried to update cfflib to work with Python3.

When I said "no one else", I meant "no one apart from TVB" seems to be using cfflib. And so, if they could adopt cfflib and keep it in a functional state, that would be good.

This is what @liadomide, which seems clear enough?

Yes, we are reading CFF files, but in tvb-framework.
If this becomes too difficult to maintain, we could "adopt" in tvb codebase only the parts we need for reading CFF files.
https://github.com/the-virtual-brain/tvb-framework/blob/master/tvb/adapters/uploaders/cff_importer.py

To summarise:

  • TVB users do need cfflib---it cannot be dropped.
  • cfflib needs to be updated to work in python3.
  • until that is done, TVB cannot be included in NeuroFedora (since we do not want to provide unmaintained python2 code).

In the meantime, would you have any hints on the failing test please?

@sanjayankur31
Copy link
Author

Hello, any updates here please?

@unidesigner
Copy link
Contributor

Hi, if TVB users really need/use cfflib, they should be able to provide a fix or adopt their codebase to only need the parts they need for reading the cffiles. Cfflib is not maintained anymore and including it into NeuroFedora would give the impression that it is, it's better to bitte the bullet now.

@liadomide
Copy link

I agree, including cfflib in NeuroFedora would send the wrong message.
We (from TVB) can adopt the parts from cfflib that are useful for our reads. But only in the future releases of tvb, which will be pyhton 3 compatible. Unfortunately, I do not have an exact estimation for that. Hopefully it will be all ready at the end of 2019.

@sanjayankur31
Copy link
Author

Thanks for the confirmation, both. I guess I'll drop both cfflib and TVB from our TODO list at the moment then. We'll check TVB again next year and work on it if its ready.

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.

None yet

3 participants