Skip to content

fix: report human-readable, calendar-aware elapsed time in memleak message - #5012

Open
jasonb5 wants to merge 6 commits into
masterfrom
jasonb5/system-tests/fix-memleak-elapsed-days
Open

fix: report human-readable, calendar-aware elapsed time in memleak message#5012
jasonb5 wants to merge 6 commits into
masterfrom
jasonb5/system-tests/fix-memleak-elapsed-days

Conversation

@jasonb5

@jasonb5 jasonb5 commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Description

Fixes #5007.

The memory-leak check in CIME/SystemTests/system_tests_common.py reported elapsed model time in "days" using arithmetic that was not actually days. It subtracted two YYYYMMDD-encoded model-date stamps directly, so a 5-year run reported nonsense like 41129 days (the message mixed years ×10000, months ×100, and day-of-month).

Root cause

perf_get_memory_list stores the model date as float(YYYYMMDD) (e.g. 51231.0), and perf_check_for_memory_leak differenced the raw stamps:

elapsed_days = int(memlist[-1][0]) - int(memlist[1][0])  # 51231 - 10102 = 41129

This is cosmetic only — the pass/fail decision uses finalmem/originalmem and the tolerance ratio, both unaffected — but the message is misleading and can send developers debugging in the wrong direction.

Fix

  • Decode the YYYYMMDD stamp into (year, month, day) and report elapsed time in years/months/days, dropping zero components, e.g. memleak detected, memory went from 1499.740000 to 1578.320000 in 4 years, 11 months, 29 days.
  • Make the arithmetic calendar-aware: the case CALENDAR variable (NO_LEAP or GREGORIAN) is threaded through so a day-borrow uses the true length of the preceding month (including the Gregorian leap-year rule) rather than a flat 30-day approximation.
  • New helper _days_in_month(year, month, calendar_type).
  • Renamed the formatting helper to _format_elapsed_model_time (describes intent rather than input encoding).

Tests

  • TestFormatElapsedModelTime: singular/plural units, every component combination, day/month/year borrows, float stamps, and century-scale runs.
  • Calendar-specific cases: NO_LEAP February, Gregorian leap / non-leap / century / 400-year February, and case-insensitive calendar names.
  • New TestDaysInMonth class covering 30/31-day months and all February edge cases.
  • Updated the memleak integration test to supply CALENDAR and assert the new message.

All test_unit_system_tests.py tests pass (the 2 pre-existing test_generate_baseline failures are unrelated — they spawn a real subprocess). Doctests pass; black (22.3.0) and pylint (4.0.4) clean.

Checklist

  • My code follows the style guidelines of this project (black formatting)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that exercise my feature/fix and existing tests continue to pass
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding additions and changes to the documentation

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the MEMLEAK failure message to report human-readable, calendar-aware elapsed model time rather than subtracting raw YYYYMMDD stamps (which previously produced nonsensical “days” values). This improves diagnostic accuracy without changing the leak pass/fail criteria.

Changes:

  • Add _days_in_month and _format_elapsed_model_time helpers to decode YYYYMMDD stamps and compute elapsed years/months/days with NO_LEAP vs GREGORIAN February handling.
  • Update perf_check_for_memory_leak to use the case CALENDAR when formatting the elapsed time in the MEMLEAK message.
  • Expand unit tests to cover formatting and calendar edge cases, and update the existing memleak message assertion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
CIME/SystemTests/system_tests_common.py Adds calendar-aware elapsed-time formatting and uses it in the memleak message.
CIME/tests/test_unit_system_tests.py Converts tests to pytest-style classes and adds/updates unit tests for the new helpers and updated message.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1196 to +1210
years = y1 - y0
months = m1 - m0
days = d1 - d0

if days < 0:
# Borrow one month, using the true length of the month immediately
# preceding the end month for the case's calendar.
borrow_year, borrow_month = y1, m1 - 1
if borrow_month == 0:
borrow_year, borrow_month = y1 - 1, 12
days += _days_in_month(borrow_year, borrow_month, calendar_type)
months -= 1
if months < 0:
months += 12
years -= 1
# raw: years=0, months=1, days=-4
# borrow Feb (NO_LEAP → 28): days = -4 + 28 = 24, months = 0
assert _format_elapsed_model_time(10205, 10301) == "24 days"

jasonb5 and others added 4 commits July 22, 2026 10:35
Closes #5007. perf_check_for_memory_leak was computing elapsed time by
subtracting two raw YYYYMMDD-encoded model-date stamps, producing a
nonsensical result (e.g. 41129 "days" for a 5-year run).

Add _yyyymmdd_elapsed_str helper that decodes each YYYYMMDD stamp into
(year, month, day) components, applies 30-day borrow arithmetic
(calendar is unavailable at this call site), and returns a
human-readable string that omits zero components and uses correct
singular/plural forms.

Update perf_check_for_memory_leak to use the helper in the diagnostic
message. The pass/fail logic (memory ratio) is unchanged.

Update existing memleak tests to use realistic YYYYMMDD stamps so
they actually exercise the YYYYMMDD decode path.
Replace the fixed 30-day month-borrow approximation in the memleak
elapsed-time message with calendar-aware arithmetic. The case CALENDAR
variable (NO_LEAP or GREGORIAN) is now threaded through so month/year
borrows use the true length of the preceding month, making the reported
elapsed time exact rather than approximate.

Add _days_in_month helper and rename _yyyymmdd_elapsed_str to
_format_elapsed_model_time to describe intent rather than input encoding.
- docker/setup-qemu-action@v3 → @v4
- docker/setup-buildx-action@v3 → @v4
- docker/login-action@v3 → @v4
- docker/metadata-action@v5 → @v6
- docker/build-push-action@v6 → @v7

Also fix f-string interpolation bug in test_scheduler.py where
self._proc_pool was not wrapped in braces for interpolation.
@jasonb5
jasonb5 force-pushed the jasonb5/system-tests/fix-memleak-elapsed-days branch from a4c896c to 3d1e708 Compare July 22, 2026 17:35
jasonb5 added 2 commits July 22, 2026 10:56
A single month-borrow was insufficient for spans that cross multiple
short months (e.g. Jan 31 → Mar 1 on NO_LEAP). The day-borrow block
now loops, walking borrow_month backwards each iteration, until days
is non-negative. The months-borrow block is likewise a while loop for
defensive correctness.

Regression tests added for NO_LEAP and GREGORIAN (leap and non-leap)
variants of Jan 31 → Mar 1, and a cross-year-boundary multi-borrow case.
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.

perf_check_for_memory_leak reports nonsensical "days" value in memleak message

4 participants