Updates for Travis: new python #974

kkopachev · 2017-09-06T23:47:23Z

Updated red eye filter to work with cv2 and add test
Fix noise filter test
Updated install scripts to use opencv-python from pip
Fix skipped base_imaging handler test
Added more caching for travis

cclauss · 2017-09-06T23:57:24Z

.travis.yml

  directories:
-    - $HOME/.cache/pip
+    - /tmp/ffmpeg/bin
 sudo: false


My take is you don't need the sudo and dist lines because these are not Travis defaults and it would be a bummer to lock in trusty when Travis updates distro but this is minor stuff.

cclauss · 2017-09-07T00:02:33Z

.travis.yml

@@ -1,62 +1,54 @@
 language: python


Really cool stuff!! Just some minor comments...

cclauss · 2017-09-07T00:02:34Z

.travis.yml

-    - gfortran
-    - libopenblas-dev
-    - liblapack-dev
-    - python-pyexiv2
    - yasm
 install:
  - pip install -U --upgrade pip


Could this be --user instead of -U? Explicit is better than... I forgot to do this on an earlier PR.

kkopachev · 2017-09-07T16:32:41Z

should be all good now

marcelometal · 2017-09-07T22:36:15Z

Hi @kkopachev,

Who is the copyright holder of tests/fixtures/filters/redeye.png, could you please add this info and the license?

kkopachev · 2017-09-07T23:15:54Z

@marcelometal It's public domain. Not sure where to add that info.

marcelometal · 2017-09-07T23:30:32Z

@kkopachev, maybe here tests/fixtures/filters/README.mkd ?

kkopachev · 2017-09-07T23:54:53Z

@marcelometal done! Thanks.

masom · 2017-09-13T12:46:16Z

.travis.yml

  directories:
-    - $HOME/.cache/pip
-sudo: false
+    - /tmp/ffmpeg/bin


scorphus · 2017-09-13T16:03:38Z

Hey, @kkopachev. Can you please:

Squash commits that are related to each other (like fix commits and/or commits that undo things done by an earlier commit, such as the miniconda in and out)
Rebase the commits on top of current master (a.k.a. resolve conflicts)

Thank you very much!

kkopachev · 2017-09-13T20:39:15Z

@scorphus do you prefer a way of pulling ffmpeg off of zesty repo - requires sudo on travis, might be slow to start the test. Or preserve ffmpeg compile stage and use travis cache?

scorphus · 2017-09-13T21:49:42Z

Hey @kkopachev. Glad you're still with us! 🤗

@marcelometal's PR – #975 – that installs ffmpeg from zesty was already merged, so I guess we're good on that.

coveralls · 2017-09-14T05:43:22Z

Coverage increased (+1.5%) to 86.06% when pulling 79dc61e on kkopachev:new-travis into 2b86a4d on thumbor:master.

kkopachev · 2017-09-14T05:45:02Z

updated

scorphus

That's all I could review for now. @marcelometal, can you please chime in and give us your thoughts?

scorphus · 2017-09-14T17:28:35Z

setup.py

    "flake8",
    "yanc",
    "remotecv",
-    "pyssim",
+    "pyssim>=0.4.0",


I'm wondering why we're setting a lower threshold for pyssim here. There's nothing different in tests nor in docs on this commit. I looked briefly but could not see any relation between pyssim >= 0.4 and latest Python versions. Excuse me if I missed something.

so, this is only a list of test dependencies.
previously, scipy was manually included, but never used directly anywhere in the code. Only usage was through pyssim.
Pyssim, in it's turn, does not seem to install its dependencies automatically, up to latest release (0.4). jterrace/pyssim#24
Now that pyssim can automatically install it's own dependencies, I decided to remove explicit require of scipy, as it was not directly used and let pip's pyssim do the magic.

Oh, my bad, didn't notice that diff regarded changes made to test_require.

scorphus · 2017-09-14T17:29:38Z

setup.py

    "cairosvg>=1.0.0,<2.0.0,!=1.0.21",
    "preggy>=1.3.0",
+    "opencv-python",


Here we're making this a mandatory dependency, which is not the case for Thumbor, I believe. Only if the user decides to use such engine and configure Thumbor to do so and in such cases he/she should install the desired engine. Perhaps docs regarding this should be improved.

This is a list of test dependencies. opencv could be used in PIL engine to convert 16bit TIFFs to png.

ah, it's also used in redeye filter and detectors. And in order to test that it's all required.

kkopachev · 2017-09-14T17:43:51Z

.travis.yml

@@ -1,23 +1,23 @@
 language: python
 python:
-  - 2.7_with_system_site_packages


so, with_system_site_packages thing forces travis to use whatever python is shipped with distribution. IIRC it's 2.7.6 at the moment. Then there were some problems installing opencv... It was easier to go with virtualenv and install packages from pip instead (reason to upgrade from cv/cv2 to cv3)

This is a nice improvement.

kkopachev · 2017-09-23T00:34:54Z

Any updates here?

coveralls · 2017-09-23T00:39:09Z

Coverage increased (+1.5%) to 86.123% when pulling 6de04cb on kkopachev:new-travis into 0053c69 on thumbor:master.

cclauss

There is some good stuff. What is to be done before it is considered done?

marcelometal · 2017-09-26T22:57:29Z

.travis.yml

 before_install:
  - echo "deb http://archive.ubuntu.com/ubuntu zesty main universe" | sudo tee --append /etc/apt/sources.list
  - sudo apt-get update
  - sudo apt-get install ffmpeg -y -f
 install:
-  - pip install -U --upgrade pip
+  - pip install --upgrade pip
+  - pip install -I https://github.com/escaped/pyexiv2/archive/master.zip


Hi @kkopachev, master branch may not be completely stable at a given moment and could break Thumbor build. What do you think about installing this dependency via .deb package?

not sure I can use deb to make it automatically available in virtual env. I can reference commit hash instead.

pyexiv2 is dead anyway, not likely there will be any releases.

Currently, pyexiv2 is installed via .deb package (Travis) and the tests are running.

current travis setup uses python with system provided packages, that's why it's working.
However, not working if we don't use system python and use virtualenv: https://travis-ci.org/thumbor/thumbor/jobs/280505386#L1689

Also, if you guys care that much about something breaking the build, there are other issues as well https://github.com/thumbor/thumbor/pull/975/files#r141397089

coveralls · 2017-09-27T16:20:08Z

Coverage increased (+1.4%) to 86.026% when pulling 65a65d5 on kkopachev:new-travis into 0053c69 on thumbor:master.

coveralls · 2017-09-27T16:37:12Z

Coverage increased (+1.5%) to 86.123% when pulling 1960f92 on kkopachev:new-travis into 0053c69 on thumbor:master.

kkopachev · 2017-09-27T18:29:40Z

any more comments?

marcelometal · 2017-09-27T18:43:16Z

Hi @kkopachev, Good work!

Thank you for contributing to Thumbor!

scorphus · 2017-09-28T15:21:59Z

Merged in 2f15296 🎉 You are a rockstar, @kkopachev! 🚀 Thank you for your contributions to Thumbor!

cclauss approved these changes Sep 7, 2017

View reviewed changes

kkopachev force-pushed the new-travis branch from 3a10fe6 to 53d501a Compare September 7, 2017 16:25

masom reviewed Sep 13, 2017

View reviewed changes

.travis.yml Outdated

directories:

- $HOME/.cache/pip

sudo: false

- /tmp/ffmpeg/bin

Copy link

Contributor

masom Sep 13, 2017

Choose a reason for hiding this comment

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

🎉

kkopachev force-pushed the new-travis branch from 7fe4a7d to 79dc61e Compare September 14, 2017 05:37

scorphus requested changes Sep 14, 2017

View reviewed changes

kkopachev commented Sep 14, 2017

View reviewed changes

kkopachev added 4 commits September 22, 2017 17:33

New red eye filter based on cv2/cv3

98d90b0

Update noise filter test image to be png, so SSIM will work

1d5ecf1

Use latest Python 2.7 on Travis

40e34ea

Looks like skipped test is fixed now; uncommenting

6de04cb

kkopachev force-pushed the new-travis branch from 79dc61e to 6de04cb Compare September 23, 2017 00:34

cclauss reviewed Sep 26, 2017

View reviewed changes

marcelometal reviewed Sep 26, 2017

View reviewed changes

Fix version of pyexiv2

1960f92

kkopachev force-pushed the new-travis branch from 65a65d5 to 1960f92 Compare September 27, 2017 16:21

scorphus closed this Sep 28, 2017

kkopachev deleted the new-travis branch September 28, 2017 16:27

Kmaschta mentioned this pull request Jan 5, 2018

6.4.0 release status #1023

Closed

9 tasks

Updates for Travis: new python #974

Updates for Travis: new python #974

Conversation

kkopachev commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkopachev commented Sep 7, 2017

marcelometal commented Sep 7, 2017

kkopachev commented Sep 7, 2017 • edited

marcelometal commented Sep 7, 2017

kkopachev commented Sep 7, 2017

Choose a reason for hiding this comment

scorphus commented Sep 13, 2017

kkopachev commented Sep 13, 2017

scorphus commented Sep 13, 2017

coveralls commented Sep 14, 2017

kkopachev commented Sep 14, 2017

scorphus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkopachev commented Sep 23, 2017

coveralls commented Sep 23, 2017 • edited

cclauss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 27, 2017

coveralls commented Sep 27, 2017

kkopachev commented Sep 27, 2017

marcelometal commented Sep 27, 2017

scorphus commented Sep 28, 2017

kkopachev commented Sep 7, 2017 •

edited

coveralls commented Sep 23, 2017 •

edited