SciKit-Surgery / scikit-surgerybard

BARD: The Basic Augmented Reality Demo, used at MedICSS summer school.
https://scikit-surgerybard.readthedocs.io/
Other
7 stars 3 forks source link

Keep pyside version same as for surgeryvtk #21

Closed thompson318 closed 4 years ago

thompson318 commented 4 years ago

In GitLab by @StephenThompson on Jul 2, 2019, 12:46

thompson318 commented 4 years ago

In GitLab by @StephenThompson on Jul 2, 2019, 13:52

mentioned in commit d5fdb3a8ceba3d8888d0ef87f7c0220e45260e01

thompson318 commented 4 years ago

In GitLab by @MianAhmad on Jul 8, 2019, 15:50

After some digging I found something interesting, at least for me.

If we do pip install . the program will install packages mentioned in setup.py in Install_requires section.

If we do pip install -rrequirements.txt the program will install packages mentioned in requirements.txt.

In surgeryvtk, setup.py (Install_requires section) has PySide2<=5.11.0 whereas requirements.txt has PySide2<=5.12.0.

So I think either both files should have the same packages or packages should be only in one place may be in requirements.txt or may be in setup.py.

@MattClarkson @StephenThompson @ThomasDowrick

thompson318 commented 4 years ago

In GitLab by @MattClarkson on Jul 8, 2019, 20:39

Ask @ThomasDowrick for a definitive version number. I believe it should be 5.12.0.

Then setup.py in all projects should match requirements.txt. Im not sure of an easy way to automatically get setup.py to inherit settings from requirements.txt (or other way round).

Also - you should never be running pip install in your dev environment. You should go via tox which uses requirements.txt.

thompson318 commented 4 years ago

In GitLab by @ThomasDowrick on Jul 9, 2019, 09:44

Correct - we should use 5.12.0

Also correct that there isn't any easy way of getting requirements.txt and setup.py to match (unfortunately).

thompson318 commented 4 years ago

In GitLab by @MianAhmad on Jul 9, 2019, 10:53

Thanks guys.
I changed PySide2 version and now both setup.py and requirements.txt have version PySide2<=5.12.0 in scikit-surgeryvtk and also in scikit-surgerybard.
Merge request created to @ThomasDowrick for scikit-surgeryvtk and to @StephenThompson for scikit-surgerybard.

thompson318 commented 4 years ago

In GitLab by @MianAhmad on Jul 9, 2019, 11:05

I noticed a relevant issue.

I guess that tox is checking requirements.txt during tox and not setup.py.

To check the setup.py. I performed the following steps:

mkdir tmp
cd tmp
git clone git@weisslab.cs.ucl.ac.uk:WEISS/SoftwareRepositories/SNAPPY/scikit-surgeryvtk.git
virtualenv -p python3.6 env1
source env1/bin/activate
cd scikit-surgeryvtk
pip install .
# After that I did pip list
(env1) mahmad3:scikit-surgeryvtk mianasbatahmad$ pip list
Package               Version         
--------------------- ----------------
numpy                 1.16.4          
opencv-contrib-python 4.1.0.25        
pip                   19.1.1          
PySide2               5.12.0          
scikit-surgerycore    0.1.6           
scikit-surgeryimage   0.5.3           
scikit-surgeryvtk     0.8.1+8.ge331cc6
setuptools            41.0.1          
shiboken2             5.12.0          
six                   1.12.0          
vtk                   8.1.2           
wheel                 0.33.4  

# Upto here is all good.
# I went into python environment and tried to import scikit-surgeryvtk and got the error i.e.
(env1) mahmad3:scikit-surgeryvtk mianasbatahmad$ python
Python 3.6.8 (v3.6.8:3c6b436a57, Dec 24 2018, 02:10:22) 
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import scikit-surgeryvtk
  File "<stdin>", line 1
    import scikit-surgeryvtk
                 ^
SyntaxError: invalid syntax
>>> import numpy

Do we need to get rid of -?

thompson318 commented 4 years ago

In GitLab by @StephenThompson on Jul 9, 2019, 11:44

I think the import statement is wrong,

import sksurgeryvtk

should work.

thompson318 commented 4 years ago

In GitLab by @MianAhmad on Jul 9, 2019, 11:48

Thanks, yes it worked.

thompson318 commented 4 years ago

In GitLab by @MianAhmad on Jul 9, 2019, 11:48

I believe the issue can be close after PySide version update.

thompson318 commented 4 years ago

In GitLab by @StephenThompson on Jul 9, 2019, 11:50

closed