Python version of phase_stats.py calculation - #1787
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughPhase statistics now fetch all run measurements once and compute per-phase results in memory. The pipeline handles percentile interpolation, phase boundaries, derivatives, sampling rates, energy, power, carbon, custom metrics, and SCI state. Carbon-intensity provider detection and configuration names use the updated metric naming convention. Phase-stat rebuilding derives SCI custom metric names from each run’s 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/phase_stats.py (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: trailing space in status message / prefer logging.
print('Fetched data for ', args.run_id)emits"Fetched data for <id>"(double space from the comma separator). Minor cosmetic nit; consider an f-string or the project's logger for consistency.♻️ Optional tweak
- print('Fetched data for ', args.run_id) + print(f'Fetched data for {args.run_id}')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faa34ad5-df0f-4d1c-848d-42f244669fae
📒 Files selected for processing (2)
lib/phase_stats.pytools/phase_stats.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
[skip ci]
|
@ribalba PR now ready for review. Since it is in the crucial calculation part please give it an extra pair of eyes |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
Eco CI Output - Old Energy EstimationEco CI Output [RUN-ID: 30005732884]:
🌳 CO2 Data: Total cost of whole PR so far: |
|||||||||||||||||||||||||||||||||||
[skip ci] * main: (fix): Custom metrics now allows valid floats and even negative numbers also (ci): Added Eco-CI measurement after setup (test-fix): Downgrading python version as 3.15 did not exist yet Upgraded Python version in CI/CD (feat): Making GitHub Action two step to debug setup vs. run better Bump actions/setup-python from 6 to 7 in /.github/actions/gmt-pytest (#1786) Bump fastapi from 0.139.0 to 0.139.2 (#1780) (feat): All config files are now in a config folder instead of clobbering the root dir and docker dir (#1793) Updated Cloud Energy Bump pandas from 3.0.3 to 3.0.5 (#1790) Bump aiohttp from 3.14.2 to 3.14.3 (#1792) Bump cachetools from 7.1.4 to 7.1.5 (#1791) (fix): 0 Values in CPU Utilization Provider for linux do not lead to Floating Point Exception anymore
…play / rebuild tools
…by list to be future proof
* main: Test speedup (#1782) Bump fastapi from 0.140.13 to 0.141.1 (#1805) Bump uvicorn from 0.51.0 to 0.52.0 (#1804) CLAUDE.md shall be auto updated by Agent Carbondb backfill and copy over rework (#1802) (hotfix): Email jobs are now handled separately and not as bulk (rework): Making some more columns NOT NULL where we enforce values to make indexes cleaner and simplify mental overhead for data strucutres CLAUDE.md added as symlink of AGENTS.md to make CLAUDE Code also read it by default (test-fix): Race condition with outdated commit-hash variable Bump fastapi from 0.139.2 to 0.140.13 (#1799) Bump tqdm from 4.69.0 to 4.70.0 (#1798) Bump cachetools from 7.1.5 to 7.1.6 (#1794) Time to filter should wait for backfill_geo to insert location data, which runs typically on a 10 minute interval Reapply "(Update): Playwright v1.62.0" - Release has been pulled and is not available anymore Revert "(Update): Playwright v1.62.0" - Release has been pulled and is not available anymore (Update): Playwright v1.62.0 (fix): Pandas Dataframe must guard against non int data since customers can supply arbitrary regexes Revert "(fix): Custom metrics now allows valid floats and even negative numbers also"
|
Merged in main to use now speedier tests. Then will merge in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/phase_stats.py (1)
293-302: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnresolved:
KeyErrorwhen a metric has zero recorded values.
metricscomes frommeasurement_metrics(all metrics registered for the run), butmetric_time_seriesis only populated frommeasurement_valuesrows. A metric with zero recorded values is absent frommetric_time_series, sometric_time_series[measurement_metric_id][0]at line 294 raisesKeyErrorand aborts the entire run's phase-stat build. The added comment ("can fail if metric does not exist. This should never be. Thus we simply crash") restates the same assumption a prior review already showed can be false — a provider can register a metric row and still record zero values (fast phase, provider failure after registration, etc.). The previous per-(phase, metric) SQL simply returned no rows in that case; this is a regression from crash-safe to crash-prone.🛡️ Proposed fix
- times = metric_time_series[measurement_metric_id][0] # can fail if metric does not exist. This should never be. Thus we simply crash - values = metric_time_series[measurement_metric_id][1] # can fail if metric does not exist. This should never be. Thus we simply crash + times, values = metric_time_series.get(measurement_metric_id, ([], []))
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa0d998c-d4dd-42c5-830e-4f0ba9fbcfb7
📒 Files selected for processing (6)
config.yml.examplefrontend/js/helpers/config.js.examplelib/phase_stats.pymetric_providers/carbon/intensitylevel/electricitymaps/machine/provider.pytools/phase_stats.pytools/rebuild_phase_stats.py
|
Eco CI Output [RUN-ID: 30625199342]:
🌳 CO2 Data: Total cost of whole PR so far: |
||||||||||||||||||||||||||||||||||||||||
* main: (tests): Moving Daily GitHub Actions Job also to Blacksmith (feat): Better stdout passing through in cluster errors Python version of phase_stats.py calculation (#1787) Test speedup (#1782) Bump fastapi from 0.140.13 to 0.141.1 (#1805) Bump uvicorn from 0.51.0 to 0.52.0 (#1804) CLAUDE.md shall be auto updated by Agent Carbondb backfill and copy over rework (#1802) (hotfix): Email jobs are now handled separately and not as bulk (rework): Making some more columns NOT NULL where we enforce values to make indexes cleaner and simplify mental overhead for data strucutres CLAUDE.md added as symlink of AGENTS.md to make CLAUDE Code also read it by default (test-fix): Race condition with outdated commit-hash variable Bump fastapi from 0.139.2 to 0.140.13 (#1799) Bump tqdm from 4.69.0 to 4.70.0 (#1798)
* main: (711 commits) (fix): Chmod should not execute Bump cachetools from 7.1.6 to 7.1.7 (#1811) (rework): Jobs.py does not take CLI arguments anymore (#1812) Bump uvicorn from 0.52.0 to 0.52.1 (#1810) Fix repo URL generation to handle GitLab, Bitbucket, SSH, and local repos in the UI (#1783) (hotfix): Removing Docker Gunicorn Cache to work with Blacksmith. Fix maybe not final depending on performance Bump redis from 8.0.1 to 8.1.0 (#1807) Bump pytest-xdist from 3.6.1 to 3.8.0 (#1808) (rework): Removing non functional id primary key on measurement_values and introducing actual used compund primary key that enables easier sort (test-fix): Using new Electriciy Maps Token after their Sandbox deprecation (tests): Moving Daily GitHub Actions Job also to Blacksmith (feat): Better stdout passing through in cluster errors Python version of phase_stats.py calculation (#1787) Test speedup (#1782) Bump fastapi from 0.140.13 to 0.141.1 (#1805) Bump uvicorn from 0.51.0 to 0.52.0 (#1804) CLAUDE.md shall be auto updated by Agent Carbondb backfill and copy over rework (#1802) (hotfix): Email jobs are now handled separately and not as bulk (rework): Making some more columns NOT NULL where we enforce values to make indexes cleaner and simplify mental overhead for data strucutres ...
2 participants
This PR speeds up the processing for the phase stats by moving calculation out of the DB hotpath.
Analysis
We first assumed the bottle neck was the SQL queries, which became quite complex, or the big database with the big index (In our live DB of ~ 100 GB of raw measurement values)
However the slow down was the network connects. We make a separate SQL query per metric and phase. This can sum up to 6 x 30 calls easily. With setup and teardown each network call takes about 1 second.
Now we only do one big query to fetch all the values resulting in a 100x speedup
Summary
MAX_POSTGRES_BIGINTfor PostgreSQL-compatible integer handling.