-
Notifications
You must be signed in to change notification settings - Fork 2
Feature add data model path configuration support #9
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
Conversation
edoardolegnaro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can merge after changing typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds configurable data/model download paths via environment-driven settings, and introduces pre-commit tooling updates.
Changes:
- Added
app/config.pysettings fordata_path/models_pathand wired them into model/data download flows. - Updated model download utilities to use a configurable cache directory instead of a fixed
.cache. - Added
.pre-commit-config.yamland adjusted Docker/requirements/docs accordingly.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Adjusts dependencies (notably FastAPI extras) and git dependency for arccnet. |
| app/model_utils.py | Refactors model download/extract caching to use a configurable path. |
| app/mcintosh_model.py | Uses settings.models_path when downloading the McIntosh model weights. |
| app/hale_model.py | Uses settings.models_path when downloading the Hale model weights. |
| app/fulldisk_model.py | Uses settings.models_path when downloading YOLO weights. |
| app/config.py | Introduces Settings with env-configurable data_path and models_path. |
| app/classify.py | Passes configured data path into Fido.fetch() for magnetogram downloads. |
| app/api.py | Formatting/line-wrapping changes to endpoint definitions. |
| app/main.py | Minor formatting consistency (quotes, trailing commas). |
| README.md | Documents configuration via env vars and default paths. |
| Dockerfile | Switches base image tag and changes dependency installation steps; exposes port 80. |
| .pre-commit-config.yaml | Adds ruff/format/isort/codespell and basic pre-commit hooks. |
| .gitignore | Normalizes .idea entry formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RUN pip install --no-cache-dir --upgrade -r /code/requirements.txt | ||
|
|
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependencies are installed twice: first via pip install -r requirements.txt, then again via pip install ... -r /code/requirements.txt. This adds build time and can hide caching/issues. Drop the redundant second install step (or keep only one consistent installation path).
| RUN pip install --no-cache-dir --upgrade -r /code/requirements.txt |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Closes #6 and also added pre-commit configuration but it not include in CI yet.