`fre app regrid` edits for `fre-workflows` functionality by ilaflott · Pull Request #669 · NOAA-GFDL/fre-cli

ilaflott · 2025-10-31T15:46:50Z

No description provided.

…acy behavior to be fulfilled

codecov · 2025-10-31T15:59:24Z

Codecov Report

❌ Patch coverage is 92.77108% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.09%. Comparing base (76df41a) to head (5ec043a).

Files with missing lines	Patch %	Lines
fre/app/regrid_xy/regrid_xy.py	92.77%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
+ Coverage   84.98%   85.09%   +0.11%     
==========================================
  Files          68       68              
  Lines        4523     4597      +74     
==========================================
+ Hits         3844     3912      +68     
- Misses        679      685       +6

Flag	Coverage Δ
unittests	`85.09% <92.77%> (+0.11%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR enhances the fre app regrid functionality to support fre-workflows by adding extensive logging, modifying function signatures to support absolute paths, implementing a workaround for legacy fre-nctools behavior, and improving code formatting and documentation.

Adds comprehensive debug and info logging throughout the regridding workflow
Modifies get_input_file() to accept an input_dir parameter and return absolute paths
Implements a symlink workaround for legacy fre-nctools datetime requirements
Refactors test code with proper formatting, docstrings, and PEP 8 compliance

Reviewed Changes

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

File	Description
fre/app/regrid_xy/regrid_xy.py	Adds extensive logging, modifies function signatures for absolute paths, implements legacy fre-nctools symlink workaround, updates fregrid command construction
fre/app/regrid_xy/tests/test_regrid_xy.py	Reorganizes imports per PEP 8, adds docstrings to test functions, updates tests to pass `input_dir` parameter, improves formatting
fre/app/regrid_xy/tests/generate_files.py	Adds module docstring, reformats code with proper indentation, adds docstrings to all functions, improves PEP 8 compliance

Comments suppressed due to low confidence (1)

fre/app/regrid_xy/regrid_xy.py:108

'except' clause does nothing but pass and there is no explanatory comment.

        except FileExistsError:

Copilot · 2025-11-13T12:56:24Z

+        # what in the ever loving demon magic is going on here???
+        try:
+            grid_spec_dt_symlink.symlink_to(Path(grid_spec))
+        except FileExistsError:
+            pass
+
+        # i am sorry python gods, i have failed out
+        if grid_spec_dt_symlink.exists():
+            fre_logger.warning('great? how did this happen?')
+
+        # continued inexplicable success.
+        if grid_spec_dt_symlink.is_symlink():
+            fre_logger.warning('symlink created: %s', grid_spec_dt_symlink)
+        else:
+            raise Exception('problem with accounting for legacy fregrid/fre-nctools behavior, symbolic link creation '
+                          'as-intended-side-effect failed. consult life choices.')



[nitpick] The comments "what in the ever loving demon magic is going on here???", "i am sorry python gods, i have failed out", "great? how did this happen?", and "continued inexplicable success" are unprofessional and unclear. These should be replaced with technical explanations of the symlink behavior or removed if the code is working as intended. Additionally, the exception message "consult life choices" is inappropriate for production code.

Suggested change

# what in the ever loving demon magic is going on here???

try:

grid_spec_dt_symlink.symlink_to(Path(grid_spec))

except FileExistsError:

pass

# i am sorry python gods, i have failed out

if grid_spec_dt_symlink.exists():

fre_logger.warning('great? how did this happen?')

# continued inexplicable success.

if grid_spec_dt_symlink.is_symlink():

fre_logger.warning('symlink created: %s', grid_spec_dt_symlink)

else:

raise Exception('problem with accounting for legacy fregrid/fre-nctools behavior, symbolic link creation '

'as-intended-side-effect failed. consult life choices.')

# Attempt to create a symbolic link for legacy fre-nctools compatibility.

try:

grid_spec_dt_symlink.symlink_to(Path(grid_spec))

except FileExistsError:

# Symlink already exists; proceed.

pass

# Check if the symlink exists after attempted creation.

if grid_spec_dt_symlink.exists():

fre_logger.warning('Symlink exists: %s', grid_spec_dt_symlink)

# Confirm that the symlink was created successfully.

if grid_spec_dt_symlink.is_symlink():

fre_logger.warning('Symlink created: %s', grid_spec_dt_symlink)

else:

raise Exception(

'Failed to create symbolic link for legacy fregrid/fre-nctools behavior. '

'Symbolic link creation did not succeed. Please check file permissions and whether the symlink already exists.'

)

Copilot · 2025-11-13T12:56:25Z

+        for source in component["sources"]:
+            history_file = source["history_file"]
+            for i in range(1, ntiles+1):
+                #print(f"input_dir is {input_dir}") # input dir is /home/Ian.Laflotte/Working/fre-cli/test_inputs


Commented-out debug print statement should be removed. Leftover debug code should not remain in the codebase.

Suggested change

#print(f"input_dir is {input_dir}") # input dir is /home/Ian.Laflotte/Working/fre-cli/test_inputs

Copilot · 2025-11-13T12:56:25Z


-  ppyaml = {}
-  ppyaml["name"] = yamlfile
+    ppyaml= {}


Inconsistent spacing around assignment operator. Should be ppyaml = {} instead of ppyaml= {}.

Suggested change

ppyaml= {}

ppyaml = {}

Copilot · 2025-11-13T12:56:25Z


-  generate_files.make_grid_spec()
-  mosaic_file.touch()
+    datadict=dict(grid_spec=grid_spec, inputRealm="ocean")


Missing space after assignment operator. Should be datadict = dict(...) instead of datadict=dict(...).

Suggested change

datadict=dict(grid_spec=grid_spec, inputRealm="ocean")

datadict = dict(grid_spec=grid_spec, inputRealm="ocean")

Copilot · 2025-11-13T12:56:26Z

+    fre_logger.debug('checking if %s is a tar file...', pp_grid_spec_tar)
    if tarfile.is_tarfile(pp_grid_spec_tar):
+
+        fre_logger.debug('it is a tar file! attempting top open %s', pp_grid_spec_tar)


Typo: "attempting top open" should be "attempting to open".

Suggested change

fre_logger.debug('it is a tar file! attempting top open %s', pp_grid_spec_tar)

fre_logger.debug('it is a tar file! attempting to open %s', pp_grid_spec_tar)

Copilot · 2025-11-13T12:56:26Z

    :type datadict: dict
    :param source: history file type
    :type source: str
+    :param input_dir: input file directory


Missing type annotation for the newly added input_dir parameter. The docstring documents it but the function signature should include : str type hint for consistency with other parameters.

Copilot · 2025-11-13T12:56:26Z

+    # assert False
+


Commented-out assertion # assert False should be removed. Commented-out debug code should not be left in production code.

Suggested change

# assert False

Copilot · 2025-11-13T12:56:26Z

        with tarfile.open(pp_grid_spec_tar, "r") as tar:
+
+            fre_logger.debug('opened! about to extract all from opened tar file object into %s', os.getcwd())
            tar.extractall()


Using tar.extractall() without a filter is unsafe as it can lead to path traversal attacks. Consider using tar.extractall(filter='data') (Python 3.12+) or validate member paths before extraction to prevent malicious tar archives from writing files outside the intended directory.

Copilot · 2025-11-13T12:56:27Z

        datadict["input_date"] = input_date[:8]
+        fre_logger.info( 'datadict is %s', pprint.pformat(datadict) )
+
+        datadict["grid_spec"], grid_spec_symlink = get_grid_spec(datadict)


Variable grid_spec_symlink is not used.

Suggested change

datadict["grid_spec"], grid_spec_symlink = get_grid_spec(datadict)

datadict["grid_spec"] = get_grid_spec(datadict)[0]

ilaflott · 2026-01-21T20:57:39Z

no appetite and this branch/PR served it's purpose helping me debug the postprocessing workflow. closing

ilaflott added 6 commits October 31, 2025 11:26

verbosity updates for fre app regrid

40a1e1c

fix logging statement that caused a TypeError

2e711c1

resolve paths to full ones and add a few more error checks on the fly

21182fc

regrid tweaks

6d73be2

add black magic symbolic link exception catch/pass to allow awful leg…

3be10fd

…acy behavior to be fulfilled

hail mary: make the associated file dir the input dir

5ec043a

ilaflott self-assigned this Oct 31, 2025

ilaflott and others added 3 commits October 31, 2025 16:17

pylint driven feedback

64a97f0

Merge branch 'main' into regrid_xy_tweaks_only

8c040bb

Merge branch 'main' into regrid_xy_tweaks_only

634cf02

ilaflott requested a review from Copilot November 13, 2025 12:51

Copilot started reviewing on behalf of ilaflott November 13, 2025 12:52 View session

Copilot finished reviewing on behalf of ilaflott November 13, 2025 12:54

Copilot AI reviewed Nov 13, 2025

View reviewed changes

Merge branch 'main' into regrid_xy_tweaks_only

c8f8499

ilaflott closed this Jan 21, 2026

ilaflott deleted the regrid_xy_tweaks_only branch January 21, 2026 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fre app regrid` edits for `fre-workflows` functionality#669

`fre app regrid` edits for `fre-workflows` functionality#669
ilaflott wants to merge 10 commits intomainfrom
regrid_xy_tweaks_only

ilaflott commented Oct 31, 2025

Uh oh!

codecov Bot commented Oct 31, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

Copilot AI Nov 13, 2025

Uh oh!

ilaflott commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	datadict=dict(grid_spec=grid_spec, inputRealm="ocean")
	datadict = dict(grid_spec=grid_spec, inputRealm="ocean")

	fre_logger.debug('it is a tar file! attempting top open %s', pp_grid_spec_tar)
	fre_logger.debug('it is a tar file! attempting to open %s', pp_grid_spec_tar)

	datadict["grid_spec"], grid_spec_symlink = get_grid_spec(datadict)
	datadict["grid_spec"] = get_grid_spec(datadict)[0]

Conversation

ilaflott commented Oct 31, 2025

Uh oh!

codecov Bot commented Oct 31, 2025

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

ilaflott commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants