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.

Sign up for GitHub

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

Jump to bottom

Save masks hook #35

Merged
merged 7 commits into from
Nov 5, 2018
Merged

Save masks hook #35

merged 7 commits into from
Nov 5, 2018

Conversation

gdynusa
Copy link
Contributor

@gdynusa gdynusa commented Oct 31, 2018

No description provided.

@gdynusa gdynusa requested a review from petrbel October 31, 2018 15:53
@gdynusa gdynusa force-pushed the save-masks-hook branch 3 times, most recently from 27a1b39 to 7e7e8eb Compare October 31, 2018 16:55
FloopCZ
FloopCZ reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@FloopCZ FloopCZ left a comment

Choose a reason for hiding this comment

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

Thank you @gdynusa for the migration.

emloop/hooks/save_masks.py Outdated
import collections
import logging

import cv2
Copy link
Contributor

@FloopCZ FloopCZ Oct 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Please only try this import and if it fails, produce an exception with a reasonable message informing the user that this is an optional and requires OpenCV. Make sure that emloop works even when opencv is not installed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FloopCZ so the package & dependency installation in CircleCI config should stay there? And if the import produces exception than tests will fail so should I skip them in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, basically

  1. catch ImportError as @FloopCZ suggested
  2. in one test-container: install opencv in CircleCI and run full tests (all must succeed)
  3. in second test-container: don't install opencv and run all tests without those requiring opencv (all must succeed)

@FloopCZ
Copy link
Contributor

FloopCZ commented Oct 31, 2018

Also the package is gtk3, not gtk-3.

@gdynusa gdynusa changed the title Save masks hook [WIP] Save masks hook Nov 1, 2018
@gdynusa gdynusa changed the title [WIP] Save masks hook Save masks hook Nov 1, 2018
@gdynusa
Copy link
Contributor Author

gdynusa commented Nov 1, 2018

@FloopCZ @petrbel wasn't sure if we want coverage, doc and deployment to be dependent on those new test containers, as well as nightly build of those, so I didn't include it for now

FloopCZ
FloopCZ reviewed Nov 1, 2018
View reviewed changes
.circleci/config.yml Outdated
run:
name: Install opencv dependencies on Arch Linux.
command: |
pacman -Syu --noconfirm opencv opencv-samples hdf5 gtk3
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need opencv-samples ?

Copy link
Contributor Author

@gdynusa gdynusa Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I might try without it and we'll see, this configuration was what I found on stackoverflow discussion

.circleci/config.yml Outdated
name: Install opencv dependencies on Ubuntu.
command: |
export DEBIAN_FRONTEND=noninteractive
apt-get install -y --force-yes opencv-data libopencv-dev python3-opencv
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need opencv-data ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this line from blocks config, as above, I'll try without it and we'll see

.circleci/config.yml Outdated
@@ -126,6 +175,9 @@ workflows:
- test_ubuntu_latest
- test_ubuntu_rolling
- test_archlinux
- test_ubuntu_latest_opencv
- test_ubuntu_rolling_opencv
- test_archlinux_opencv
Copy link
Contributor

Choose a reason for hiding this comment

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

What about instead of installing the system all over, we would just test it without opencv, than install opencv to the same system and then test it again? (i.e., just adding two steps to the old test_ubuntu_latest etc.)

Copy link
Member

Choose a reason for hiding this comment

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

nice!

FloopCZ
FloopCZ approved these changes Nov 5, 2018
View reviewed changes
Copy link
Contributor

@FloopCZ FloopCZ left a comment

Choose a reason for hiding this comment

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

Thank you @gdynusa !

@FloopCZ FloopCZ merged commit 3975354 into dev Nov 5, 2018
@FloopCZ FloopCZ deleted the save-masks-hook branch November 5, 2018 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FloopCZ FloopCZ FloopCZ approved these changes

@petrbel petrbel Awaiting requested review from petrbel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gdynusa @FloopCZ @petrbel