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

Fix docker #66

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Fix docker #66

merged 7 commits into from
Mar 16, 2022

Conversation

MateoLostanlen
Copy link
Member

@MateoLostanlen MateoLostanlen commented Feb 3, 2022

This PR aims to solve several docker problems in the repo.
Pyro-engine Docker

  1. The Dockerfile for pyro-engine was using the wrong image of pyro-vison
  2. We have a problem with the version of pyro-vision (I will open an issue) so we have to delete the version for the moment

Web Server Docker

  1. We delete the useless lines in the Dockerfile
  2. We delete the useless huawei dependency and we don't block the version which can lead to problems later

@MateoLostanlen MateoLostanlen added the type: bug Something isn't working label Feb 3, 2022
@MateoLostanlen MateoLostanlen requested review from frgfm, fe51 and a team February 3, 2022 22:28
@MateoLostanlen MateoLostanlen self-assigned this Feb 3, 2022
Copy link
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks! Added a few questions/comments!

server/src/requirements.txt Outdated Show resolved Hide resolved
server/src/Dockerfile Show resolved Hide resolved
@MateoLostanlen MateoLostanlen changed the base branch from master to develop February 13, 2022 10:16
Copy link
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Just missing one version specifier for pyrovision I guess :)

requirements.txt Outdated
pyrovision >= 0.1.2
python-dotenv >= 0.15.0
requests>=2.25.1
pyrovision
Copy link
Member

Choose a reason for hiding this comment

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

At least a lower bound would be nice don't you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have an issue with our version number, we get pyrovision--version-.-.-0.1.2a0-288795a therefore the condition pyrovision>xx will always fail

Copy link
Member

Choose a reason for hiding this comment

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

Mmmh, perhaps I can fix this first then 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think this will fix the problem: pyronear/pyro-vision#133

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks ! But I was thinking, maybe it's better to install provision from the repo. We need all the latest updates

Copy link
Member

Choose a reason for hiding this comment

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

Those are parallel topics (means of installation vs. version specifier), but I'd say:

  • we need a version specifier for production repo 😅
  • regarding the means of installation, installing from the repo means that if a bug slips in a commit, it will have it. if we put the normal one, we can always install from the repo before running that installation and have the choice. We'll need to make a release of pyrovision soon anyway

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I see what you mean. I think we need to make a 0.1.3 release and define this version as a minimum to include recent bug fixes.

Copy link
Member Author

Choose a reason for hiding this comment

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

can you review the open_fire_v2 PR to include it in the release ?

pyrovision == 0.1.2
pyrovision
Copy link
Member

Choose a reason for hiding this comment

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

same comment here

Copy link
Member Author

Choose a reason for hiding this comment

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

same

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #66 (afe6d42) into develop (2556448) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop      #66   +/-   ##
========================================
  Coverage    68.69%   68.69%           
========================================
  Files            7        7           
  Lines          230      230           
========================================
  Hits           158      158           
  Misses          72       72           
Flag Coverage Δ
unittests 68.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2556448...afe6d42. Read the comment docs.

Copy link
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks! I'd only recommend to check that it's OK to have spaces around equal signs in the requirements.txt

requirements.txt Outdated
pyroclient@git+https://github.com/pyronear/pyro-api.git#egg=pyroclient&subdirectory=client
psutil
python-dotenv == 0.15.0
Copy link
Member

Choose a reason for hiding this comment

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

can you confirm it can accept spaces around the double equal sign?

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems to work but I will remove it just in case

@MateoLostanlen MateoLostanlen merged commit c826e7d into pyronear:develop Mar 16, 2022
MateoLostanlen added a commit that referenced this pull request Jul 2, 2022
* ci on develop (#68)

No code update here, only configuration. I take the liberty to use admin privileges to merge

* Code of conduct (#71)

* docs: Added code of conduct md file

* docs: Updated CONTRIBUTING file, related to code of conduct

* Update licence (#70)

* nothing

* nothing

* docs: Updated licence to Apache 2.0

* docs: Updated README

* docs: Updated setup.py

* chore : added script to check headers

script copied from pyro-vision and authored by frgfm

* chore : updated folders to check

* docs : updated all headers

* chore: add ci job to check headers

* style : added line at end of file to fix linter

* docs: corrected repo starting year in headers & setup

* fix version (#73)

* pyrovision from repo (#72)

* Fix docker (#66)

* use pyronear docker hub

* issue with pyro-vision version

* simplify web server docker

* fix versions for production

* remove space

* add update script (#67)

* add update script

* add crontab exemple

* add monitor runner (#74)

* change default loop interval (#75)

Co-authored-by: fe51 <55736935+fe51@users.noreply.github.com>
@MateoLostanlen MateoLostanlen deleted the fix_docker branch July 24, 2022 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants