search

CoVital-Project / Spo2_evaluation

Python script to evaluation the correctness of SpO2 estimation algorithms
18 stars 6 forks source link

Continuious integration #13

Closed MalcolmMielle closed 4 years ago

MalcolmMielle commented 4 years ago

Trying to add build test. If anyone knows how to do it, go for it

LittlePea13 commented 4 years ago

I have tried many things in my PR, none worked so far. Adding it here so that others know what was tried.

For python 3.8 the error had to do with not finding some libraries using pkg-config. I installed pkg-config and those libraries (sudo apt-get install -y libavformat-dev libavcodec-dev libavdevice-dev libavutil-dev libswscale-dev libavresample-dev libavfilter-dev). Error still persisted.

Tried with python 3.7, error switch to being related to gcc, added gcc in the apt-get install command, but error still there.

Apparently av can be a tricky thing to install with pip. Should we switch to conda? Good luck.

LittlePea13 commented 4 years ago

I think av has to be in the requirements, otherwise it won't be installed and I assume the code won't work without av.

YoniSchirris commented 4 years ago

Hey, the tests seemed to fail because of the lacking of tests, right? Currently it seems to work.

Can we merge?

gianlucatruda commented 4 years ago

I think av has to be in the requirements, otherwise it won't be installed and I assume the code won't work without av.

@LittlePea13 I used a tool called pipreqs to calculate the "top level" dependencies—the things our project directly depends on. When those are installed by pip, their subdependcies are automatically installed too. I believe av was a subdependency, so we don't need to track it :)

gianlucatruda commented 4 years ago

Hey, the tests seemed to fail because of the lacking of tests, right? Currently it seems to work.

@YoniSchirris Initially the CI was failing because it couldn't find some of the dependencies. This was due to the version numbers in the requirements file being too strict. I manually made them less strict (by switching == to >= and removing the nightly build of torchvision). That solved that issue.

Then yes, the second point of failure was that pytest couldn't find any tests to run. I added a single dummy test function in a new tests/ directory. It now finds and runs that test, so the build can pass. I've opened issue #23 to invite people to start writing tests. We should have tests for at least a few of our critical functions.

Can we merge?

Yes. I need someone to approve the changes though 😄

LittlePea13 commented 4 years ago

If you run healthwatcher, the read videos method from torchvision uses av, and it will crash if it is not installed. Since it is not a requirement for torchvision, your tool won't detect it. Nevertheless that method won't probably work anymore since we are using the dataloader, which doesn't use it, since it gives memory issues. Still, there may be some functionality from torchvision that we use in the future that may require av.

Btw, just noticed the data_loader.py file doesn't need the torchvision import.

MalcolmMielle commented 4 years ago

So just to make sure I understood the ocnversation, av is not used anymore since it's used in the healthwatcher function but has been removed from the dataloader? if that's the case I think we should remove the pytorch read video method on this branch before being able to merge it.

LittlePea13 commented 4 years ago

Basically, some torchvision functionality uses av. Right now only the healthwatcher code uses torchvision, a readvideo method that requires av. So yeah, if we change that function in healthwatcher, av won't be required for now.

Of in the future other torchvision functionality is used, we may need av.

On Mon, Apr 6, 2020, 20:26 Malcolm Mielle notifications@github.com wrote:

So just to make sure I understood the ocnversation, av is not used anymore since it's used in the healthwatcher function but has been removed from the dataloader? if that's the case I think we should remove the pytorch read video method on this branch before being able to merge it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CoVital-Project/Spo2_evaluation/pull/13#issuecomment-609960847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGHKOWJRV5SW5ZYXP6YMNHDRLIND5ANCNFSM4LXZSYTQ .