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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Refactor pyrorisks #83

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

jsakv
Copy link
Collaborator

@jsakv jsakv commented Jul 2, 2024

The PR introduces the following changes:

  • refactor:

    • Remove Heroku deployment files; the service is no longer used by Pyronear
    • Remove CML / DVC pipelines; will revisit mlops tooling later if necessary
    • Deprecate the outdated pyro_risks modules, CLI, and examples
  • chore:

    • Add poetry for packaging and dependency management
    • Add ruff for linting and formatting
    • Update docker build and config
    • Update CI workflows
    • Bump Geopandas to 1.0.1, replace fiona backend with pyogrio with speedups >5-20x
  • style: Fix type hints, formatting, and linting

  • docs: Remove deprecated modules from docs

To speed up the API deployment, the refactored version of the repository only targets Linux distributions and python 3.10; multi-platform and multi-version builds will come later. It will also allow us to assess if a mix of poetry and conda is required for installation across platforms (cf pyrovision, pyroengine).

I plan on adding unit test workflow with the next PR!

Any feedback is welcomed 馃尣

@jsakv jsakv marked this pull request as ready for review July 9, 2024 17:17
Acruve15
Acruve15 previously approved these changes Jul 10, 2024
pytest-cov = "^5.0.0"


[tool.poetry.group.app.dependencies]
Copy link
Collaborator

Choose a reason for hiding this comment

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

[QUESTION] should we name it app or api?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both can work. I named it app to match the top-level directory and the fast API app/API convention, but api is straightforward!

Copy link
Collaborator

Choose a reason for hiding this comment

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

[QUESTION] Why use both ruff and black ? Ruff can be a direct replacement for black, so it seems it is redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, thanks for catching that!

Copy link
Collaborator

@juldpnt juldpnt 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 for this PR !! I have a little question, I'm wondering if you chose Python 3.10 for technical reasons/compatibility? The 3.11 (or even 3.12) would give many QOLs for typing, interpreter errors, etc.

@jsakv
Copy link
Collaborator Author

jsakv commented Jul 11, 2024

Thank you for this PR !! I have a little question, I'm wondering if you chose Python 3.10 for technical reasons/compatibility? The 3.11 (or even 3.12) would give many QOLs for typing, interpreter errors, etc.

Fair question; the main concern was to avoid incompatibilities and reduce the surface for build errors.
For instance, geopandas supports Python 3.9+, rasterio has 3.12 wheels but is not being tested (CI) on Python 3.12, and pyogrio has had compatibility issues with Python 3.12 recently.

These few points are enough to rule out Python 3.12 preventively IMHO, and on the other sidePython 3.9 is approaching its end of life in ~ one year.

The package is compatible with Python 3.10 and 3.11, but I went with the 3.10 version for the docker image and the CI. The idea is to introduce relevant multi-platform and multi-version builds later!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants