Skip to content

Add log_and_raise helper to merge fre_logger.error and raise steps#831

Draft
Copilot wants to merge 10 commits intomainfrom
copilot/merge-fre-logger-error-raise
Draft

Add log_and_raise helper to merge fre_logger.error and raise steps#831
Copilot wants to merge 10 commits intomainfrom
copilot/merge-fre-logger-error-raise

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 3, 2026

Describe your changes

Adds a log_and_raise() helper to fre/__init__.py that combines fre_logger.error() and raise into a single call, eliminating repetitive and duplicated error text across the codebase. All raise ExcType(msg) statements throughout the codebase have been converted to use this helper.

Helper function (fre/__init__.py):

  • Annotated as -> NoReturn so pylint and type checkers treat calls to it as control flow terminators (just like raise)
  • Logs at ERROR via fre_logger with stacklevel=2 so log output attributes the correct caller, not log_and_raise itself
  • Always passes stack_info=True so the call-site stack trace appears in every configured handler, including log files set up via fre -l
  • Passes exc_info=True when exc is provided, per the Python logging HOWTO, so the caught exception's full traceback is written to every handler
  • Raises exc_type (default ValueError) with the same message
  • Supports exception chaining via exc param (raise ... from exc)
# Before (message duplicated):
fre_logger.error(f"input dir {d} does not exist")
raise OSError(f"input dir {d} does not exist")

# After:
log_and_raise(f"input dir {d} does not exist", OSError)

# With exception chaining:
except KeyError as e:
    log_and_raise("update failed", KeyError, exc=e)

Conversion patterns applied across ~44 files:

  • Simple raises: raise ExcType(msg)log_and_raise(msg, ExcType)
  • Chained exceptions: raise ExcType(msg) from exclog_and_raise(msg, ExcType, exc=exc)
  • Duplicate message: removed redundant fre_logger.error and replaced both with log_and_raise
  • Different context log + raise message: kept fre_logger.error for context, replaced raise with log_and_raise
  • Bare raise (no-arg re-raises in except blocks) — left unchanged
  • Abstract method raise NotImplementedError() in timeAverager.py — left unchanged

Traceback logging for fre -l log files:

A key motivation for this helper is ensuring Traceback info reaches log files specified via fre -l ./logger_output.log. Per Python logging best practices (PEP 282, logging HOWTO):

  • stack_info=True is always passed, so the call-site stack trace (Stack (most recent call last):) appears in every handler
  • exc_info=True is passed when exc is given, so the caught exception's traceback (Traceback (most recent call last):) appears in every handler

Two new tests (test_log_and_raise_stack_trace_in_log_file, test_log_and_raise_traceback_in_log_file) simulate fre -l ./logger_output.log by attaching a FileHandler and verify:

  1. Stack trace info appears in the log file for simple raises
  2. Caught exception traceback and the original exception message appear in the log file for chained raises

Files converted (representative list): app/helpers.py, app/remap_pp_components/, app/mask_atmos_plevel/, app/generate_time_averages/, app/regrid_xy/, pp/checkout_script.py, pp/run_script.py, pp/trigger_script.py, pp/nccheck_script.py, pp/status_script.py, pp/histval_script.py, pp/rename_split_script.py, pp/split_netcdf_script.py, pp/ppval_script.py, pp/validate_script.py, pp/install_script.py, pp/configure_script_yaml.py, make/ (multiple files), make/gfdlfremake/ (multiple files), cmor/cmor_helpers.py, cmor/cmor_mixer.py, cmor/cmor_finder.py, cmor/cmor_config.py, cmor/cmor_yamler.py, yamltools/ (multiple files), run/frerun.py, check/frecheck.py

Issue ticket number and link (if applicable)

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback
  • No print statements; all user-facing info uses logging module

Copilot AI linked an issue Apr 3, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Refactor logger error handling to merge with raise steps Add log_and_raise helper to merge fre_logger.error and raise steps Apr 3, 2026
Copilot AI requested a review from ilaflott April 3, 2026 05:13
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 51.72414% with 112 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.35%. Comparing base (836559e) to head (3b99405).

Files with missing lines Patch % Lines
fre/cmor/cmor_mixer.py 36.84% 12 Missing ⚠️
fre/make/gfdlfremake/platformfre.py 9.09% 10 Missing ⚠️
fre/yamltools/info_parsers/cmor_info_parser.py 11.11% 8 Missing ⚠️
fre/app/remap_pp_components/remap_pp_components.py 41.66% 7 Missing ⚠️
fre/app/regrid_xy/regrid_xy.py 14.28% 6 Missing ⚠️
fre/pp/ppval_script.py 16.66% 5 Missing ⚠️
fre/yamltools/combine_yamls_script.py 16.66% 5 Missing ⚠️
fre/yamltools/info_parsers/analysis_info_parser.py 16.66% 5 Missing ⚠️
fre/yamltools/info_parsers/pp_info_parser.py 28.57% 5 Missing ⚠️
...p/generate_time_averages/frenctoolsTimeAverager.py 33.33% 4 Missing ⚠️
... and 21 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #831      +/-   ##
==========================================
+ Coverage   84.12%   84.35%   +0.23%     
==========================================
  Files          71       71              
  Lines        4944     4974      +30     
==========================================
+ Hits         4159     4196      +37     
+ Misses        785      778       -7     
Flag Coverage Δ
unittests 84.35% <51.72%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
fre/__init__.py 100.00% <100.00%> (ø)
fre/app/generate_time_averages/cdoTimeAverager.py 100.00% <100.00%> (ø)
fre/app/mask_atmos_plevel/mask_atmos_plevel.py 100.00% <100.00%> (ø)
fre/cmor/cmor_config.py 97.56% <100.00%> (+0.03%) ⬆️
fre/cmor/cmor_finder.py 100.00% <100.00%> (ø)
fre/cmor/cmor_helpers.py 93.68% <100.00%> (-0.11%) ⬇️
fre/cmor/cmor_yamler.py 100.00% <100.00%> (ø)
fre/make/create_compile_script.py 97.91% <100.00%> (+0.04%) ⬆️
fre/make/create_docker_script.py 100.00% <100.00%> (ø)
fre/make/make_helpers.py 100.00% <100.00%> (ø)
... and 32 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 836559e...3b99405. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ilaflott
Copy link
Copy Markdown
Member

ilaflott commented Apr 3, 2026

@copilot find other locations raise, and make sure they log the error too, use the new approach.

@ilaflott ilaflott marked this pull request as ready for review April 3, 2026 06:07
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 3, 2026

@copilot find other locations raise, and make sure they log the error too, use the new approach.

Done in commit 4930028. Converted all raise ExcType(msg) statements across 44 files to use log_and_raise. Patterns handled:

  • Simple raises: raise ExcType(msg)log_and_raise(msg, ExcType)
  • Chained exceptions: raise ExcType(msg) from exclog_and_raise(msg, ExcType, exc=exc)
  • Same message duplicated: removed the redundant fre_logger.error and replaced both with log_and_raise
  • Different context + raise messages: kept the fre_logger.error for context, replaced the raise with log_and_raise
  • Bare raise (re-raises in except blocks) — left unchanged
  • Abstract method raise NotImplementedError() in timeAverager.py — left unchanged

@ilaflott
Copy link
Copy Markdown
Member

ilaflott commented Apr 3, 2026

@copilot updating the raise statements may have violated the assumptions of certain tests. spin up your own conda environment with an editable pip install, and tackle the testing failures one-by-one

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 3, 2026

@copilot updating the raise statements may have violated the assumptions of certain tests. spin up your own conda environment with an editable pip install, and tackle the testi...

Investigated CI failures in commit 05ea559. The pytest step was passing — the only failures were pylint E0606: possibly-used-before-assignment errors in several files (combine.py, wrapper.py, rename_split_script.py, remap_pp_components.py, ppval_script.py, cmor_helpers.py, split_netcdf_script.py).

Root cause: replacing raise ExcType(msg) with log_and_raise(msg, ExcType) breaks pylint's control flow analysis. Pylint knows raise terminates a branch, but it doesn't know that log_and_raise always raises — so it flagged variables assigned only in non-else branches as "possibly unassigned."

Fix: added -> NoReturn return type annotation to log_and_raise in fre/__init__.py. Pylint now treats calls to it as control flow terminators, the same as bare raise. Pylint now exits with code 0 (score 8.56/10).

@ilaflott ilaflott added new functioning New feature or request clean up labels Apr 3, 2026
Copilot AI and others added 7 commits April 6, 2026 15:08
Replace bare raise ExcType(msg) statements with log_and_raise(msg, ExcType)
calls, which logs the error before raising. This ensures all raised exceptions
are consistently logged via fre_logger.error.

Changes:
- Add 'from fre import log_and_raise' imports where missing
- Convert raise ValueError/TypeError/FileNotFoundError/FileExistsError/
  IOError/RuntimeError/OSError/Exception/NotImplementedError/KeyError/
  NameError to log_and_raise equivalents
- For 'raise ExcType(msg) from exc' patterns, use log_and_raise(msg, ExcType, exc=exc)
- For patterns where fre_logger.error + raise had the same message, collapse
  into single log_and_raise call
- For patterns where fre_logger.error + raise had different messages, keep
  fre_logger.error and replace raise with log_and_raise
- Preserve all bare re-raises (raise with no arguments)
- Preserve NotImplementedError in abstract methods in timeAverager.py
- Collapse multi-line raise statements into single log_and_raise calls
- Convert multi-argument ValueError to single string message log_and_raise calls

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Make exception type explicit in all log_and_raise calls where ValueError
was being raised (matching the original raise ValueError statements).
Also fix a code structure issue in combine.py where the if/elif frequency
blocks were incorrectly nested.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
…cit ValueError to log_and_raise calls

- Restore accidentally removed interval_object = duration_parser.parse(interval)
  in combine.py form_bronx_directory_name()
- Add explicit ValueError type to all log_and_raise() calls that relied on the
  default parameter: frenctoolsTimeAverager.py, generate_time_averages.py,
  wrapper.py, mask_atmos_plevel.py, platformfre.py, targetfre.py, cmor_yamler.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Replace all raise ExcType(msg) and raise ExcType(msg) from exc patterns
with log_and_raise(msg, ExcType) and log_and_raise(msg, ExcType, exc=exc)
respectively, across:

- fre/pp/configure_script_yaml.py (already had import)
- fre/cmor/cmor_helpers.py (already had import)
- fre/cmor/cmor_mixer.py (already had import)
- fre/cmor/cmor_finder.py (added import)
- fre/cmor/cmor_config.py (added import)
- fre/yamltools/helpers.py (added import)
- fre/yamltools/info_parsers/cmor_info_parser.py (added import)
- fre/yamltools/info_parsers/compile_info_parser.py (added import)
- fre/yamltools/info_parsers/analysis_info_parser.py (added import)
- fre/yamltools/info_parsers/pp_info_parser.py (added import)
- fre/yamltools/combine_yamls_script.py (added import)

Also converts printf-style multi-arg ValueError/FileNotFoundError calls
to f-string log_and_raise equivalents.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
…ed-before-assignment errors

Agent-Logs-Url: https://github.com/NOAA-GFDL/fre-cli/sessions/6e92661a-f061-4b23-a675-7ddeb69b0a21

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
@ilaflott ilaflott force-pushed the copilot/merge-fre-logger-error-raise branch from 05ea559 to 5cf50b1 Compare April 6, 2026 19:08
Replace fre_logger.error with fre_logger.exception for better error logging.
@ilaflott ilaflott marked this pull request as draft April 6, 2026 20:00
@ilaflott
Copy link
Copy Markdown
Member

ilaflott commented Apr 6, 2026

@copilot this is subtle, make sure this jives with logging best practices according to official python documentation, and PEP practices. one of the motivations for this is to get Traceback info into a log file specified by fre -l ./logger_output.log <rest of call>. that is not happening currently- craft a test to make it so

Copilot AI and others added 2 commits April 6, 2026 20:07
…traceback tests

Per Python logging best practices (PEP 282, logging HOWTO):
- Use exc_info=True (not logger.exception) for caught exception tracebacks
- Use stack_info=True to always include call-site stack trace in log output
- Both ensure Traceback info reaches log files set up via fre -l

New tests verify that when a FileHandler is attached (simulating
fre -l ./logger_output.log):
- stack trace appears in log file for simple raises (stack_info=True)
- caught exception traceback appears in log file for chained raises
  (exc_info=True)

Agent-Logs-Url: https://github.com/NOAA-GFDL/fre-cli/sessions/73b3875d-4130-4999-9488-0dc773ca1c45

Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 6, 2026

@copilot this is subtle, make sure this jives with logging best practices according to official python documentation, and PEP practices. one of the motivations for this is to get `...

Done in 3b99405. Aligned log_and_raise with Python logging best practices per the logging HOWTO and PEP 282:

  • exc_info=True when exc is provided — the idiomatic way to include a caught exception's traceback in log output (replaces logger.exception())
  • stack_info=True always — ensures the call-site stack trace appears in every handler, including log files set up via fre -l
  • stacklevel=2 attributes the log record to the caller, not log_and_raise itself

Two new tests (test_log_and_raise_stack_trace_in_log_file, test_log_and_raise_traceback_in_log_file) simulate fre -l ./logger_output.log by attaching a FileHandler and verify:

  1. Stack (most recent call last): appears for simple raises
  2. Traceback (most recent call last): + the original exception appear for chained raises

pytest.fail("expected log record not found")


def test_log_and_raise_stack_trace_in_log_file(tmp_path):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this isn't the right testing approach- the logfile behavior is more for the CLI:
fre -l logger_output.log ... and so one should use click's CliRunner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clean up new functioning New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

can we merge fre_logger.error and raise steps ?

2 participants