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

refactor: Cleaned pyroengine and refactored project #101

Merged
merged 41 commits into from
Aug 5, 2022
Merged

Conversation

frgfm
Copy link
Member

@frgfm frgfm commented Aug 3, 2022

This PR introduces the following changes:

  • simplified pyroengine
    • deprecates pi_utils (not used)
    • moved pyroengine.engine.engine to pyroengine.core
    • added pyroengine.vision with Classifier which holds the ONNX model
  • updated tests
    • moved "test" to "tests"
    • switched from unittests to pytest to harmonize our test suite across repos
    • added exhaustive unit tests for pyroengine (only part not being tested : API interactions, because it requires to run it locally)
  • updated docs
    • updated documentation theme from rtd_theme to furo (cf. modifications in pyrovision)
  • deleted server folder
  • renamed runner as src
  • cleaned src by implementing ReolinkCamera and SystemController
  • refactored all requirements.txt and config files into pyproject.toml
  • added a Makefile
  • updated CI jobs accordingly
  • updated docker file accordingly

Basically, for the wrapper (orginially runner, now src), the only difference is:

  • previously, we needed the following keys: "model_weights"
  • now, we need: "cfg_path" (describing the preprocessing), "model_path" (path to ONNX model), "hub_repo" (fallback to download the model from HF hub)

For reviewers, I highly suggest to check the modifications by commit, it might be easier 😅

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #101 (b66b75a) into main (d829ab8) will increase coverage by 5.60%.
The diff coverage is 67.70%.

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   62.29%   67.90%   +5.60%     
==========================================
  Files           5        3       -2     
  Lines         183      162      -21     
==========================================
- Hits          114      110       -4     
+ Misses         69       52      -17     
Flag Coverage Δ
unittests 67.90% <67.70%> (+5.60%) ⬆️

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

Impacted Files Coverage Δ
pyroengine/core.py 61.48% <61.48%> (ø)
pyroengine/__init__.py 100.00% <100.00%> (ø)
pyroengine/vision.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@MateoLostanlen MateoLostanlen left a comment

Choose a reason for hiding this comment

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

Hi @fg, thanks so much for this PR! Globally LGTM. Several sticking points though:

  1. This PR must be merge on develop and not main directly
  2. CIs must run on main & develop

To discuss

We never used the felix code to monitor the pi's but I think it would be nice to do so, I don't know if it's a good idea to delete it

NB: It would be better to use a fork

@frgfm
Copy link
Member Author

frgfm commented Aug 4, 2022

All good, codebase should be clean :)

@MateoLostanlen
Copy link
Member

MateoLostanlen commented Aug 4, 2022

We never used the felix code to monitor the pi's but I think it would be nice to do so, I don't know if it's a good idea to delete it

NB: It would be better to use a fork

Thanks for the update, what about these comments ?

@frgfm
Copy link
Member Author

frgfm commented Aug 4, 2022

Thanks for the update, what about these comments ?

Oops I didnt see the last part:

  • I removed it because it comes with dependencies (I would have kept it otherwise)
  • thought it would be better to start with a clean environment, then add it back later and see how we integrate it

Copy link
Member

@MateoLostanlen MateoLostanlen left a comment

Choose a reason for hiding this comment

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

LGTM

@frgfm frgfm merged commit 731391c into main Aug 5, 2022
@frgfm frgfm deleted the full-refacto branch August 5, 2022 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants