Fix docker version issue 126 #133
Conversation
- Add setuptools-git-versioning to build-system requirements - Add comment explaining .git folder is needed for versioning - This fixes the issue where version showed as 0.0.0 in Docker logs
|
Hi @peterdudfield |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #126 by fixing a Docker version problem and implementing backup data generation for missing GSP (Grid Supply Point) data. The solution creates fallback data by scaling national-level solar generation data proportionally based on installed capacity when specific GSP data is unavailable.
Key changes:
- Implements backup GSP data generation using national data and installed capacity ratios from the database
- Adds setuptools-git-versioning to build requirements to fix Docker versioning issues
- Tracks missing GSPs and generates proportional data when national data is available
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| solar_consumer/data/fetch_gb_data.py | Added logic to detect missing GSP data and generate backup values scaled from national data using database-sourced capacity ratios |
| pyproject.toml | Added setuptools-git-versioning dependency to build requirements to support version detection during Docker builds |
| Dockerfile | Added comment explaining that .git folder is required for setuptools-git-versioning to function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, national_row in national_df.iterrows(): | ||
| national_capacity = national_row['installedcapacity_mwp'] | ||
| if national_capacity == 0 or pd.isna(national_capacity): | ||
| continue | ||
| for location in locations: | ||
| if location.installed_capacity_mw is not None and location.installed_capacity_mw > 0: | ||
| factor = location.installed_capacity_mw / national_capacity | ||
| new_row = national_row.copy() |
There was a problem hiding this comment.
Using iterrows() is inefficient for DataFrame iteration. Consider using vectorized operations or itertuples() for better performance, especially when processing multiple missing GSPs with the full national dataset.
| for _, national_row in national_df.iterrows(): | |
| national_capacity = national_row['installedcapacity_mwp'] | |
| if national_capacity == 0 or pd.isna(national_capacity): | |
| continue | |
| for location in locations: | |
| if location.installed_capacity_mw is not None and location.installed_capacity_mw > 0: | |
| factor = location.installed_capacity_mw / national_capacity | |
| new_row = national_row.copy() | |
| for national_row in national_df.itertuples(index=False): | |
| national_capacity = national_row.installedcapacity_mwp | |
| if national_capacity == 0 or pd.isna(national_capacity): | |
| continue | |
| base_row = national_row._asdict() | |
| for location in locations: | |
| if location.installed_capacity_mw is not None and location.installed_capacity_mw > 0: | |
| factor = location.installed_capacity_mw / national_capacity | |
| new_row = base_row.copy() |
| try: | ||
| connection = DatabaseConnection(url=db_url) | ||
| with connection.get_session() as session: | ||
| locations = session.query(LocationSQL).filter(LocationSQL.gsp_id.in_(missing_gsps)).all() |
There was a problem hiding this comment.
The database query fetches all locations for missing GSPs but is executed inside the loop that processes each GSP. This query should remain outside any iteration loop to avoid potential N+1 query issues if the code structure changes.
| factor = location.installed_capacity_mw / national_capacity | ||
| new_row = national_row.copy() | ||
| new_row['solar_generation_kw'] *= factor |
There was a problem hiding this comment.
The scaling logic creates backup data by multiplying generation values by a capacity factor. Consider extracting this calculation into a separate function to improve testability and clarify the backup data generation algorithm.
|
Ill let you answer CoPilot code review, and then Ill have a look |
| @@ -1,5 +1,5 @@ | |||
| [build-system] | |||
| requires = ["setuptools>=61.0"] | |||
| requires = ["setuptools>=61.0", "setuptools-git-versioning>=2.0,<3"] | |||
There was a problem hiding this comment.
I think this is the only change needed for this PR, the other cahgnes dont relate this issue. You ok to change them
|
Ill close this for the moment, if thats ok |
2 participants
Pull Request
Description
setuptools-git-versioning>=2.0,<3to build-system requirementsFixes #126
Checklist: