Skip to content

(Closes #3038) catch parse error in frontend and test#3040

Merged
sergisiso merged 18 commits into
masterfrom
3038_parse_error_failure
Jul 28, 2025
Merged

(Closes #3038) catch parse error in frontend and test#3040
sergisiso merged 18 commits into
masterfrom
3038_parse_error_failure

Conversation

@arporter

Copy link
Copy Markdown
Member

A small PR to improve the Fortran frontend - we were falling over with a nasty stack trace when given some invalid Fortran.

@arporter arporter self-assigned this Jun 26, 2025
@arporter arporter added enhancement ready for review NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH labels Jun 26, 2025
@arporter

Copy link
Copy Markdown
Member Author

Ready for a look from either @LonelyCat124 or @sergisiso.

@codecov

codecov Bot commented Jun 26, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (98b0c7c) to head (55b5c72).
⚠️ Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3040   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files         368      368           
  Lines       51937    51992   +55     
=======================================
+ Hits        51887    51942   +55     
  Misses         50       50           

☔ 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.

@sergisiso sergisiso left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@arporter I agree in catching and reraising the fparser errors, as the fparser encapsulation should not escape the frontend files, but this change alone doesn't solve the error reported in the issue because the psyclone command uses the psyir_from_file, not the psyir_from_source method:

> psyclone ukca_mode_tracer_maps_mod.F90
Traceback (most recent call last):
  File "/home/centos/workspace/psyclone/.venv/lib/python3.13/site-packages/fparser/two/Fortran2003.py", line 271, in __new__
    return Base.__new__(cls, string, _deepcopy=_deepcopy)
           ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/centos/workspace/psyclone/.venv/lib/python3.13/site-packages/fparser/two/utils.py", line 511, in __new__
    raise NoMatchError(errmsg)
fparser.two.utils.NoMatchError: at line 177
>>>END DO


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/centos/workspace/psyclone/.venv/bin/psyclone", line 42, in <module>
    main(sys.argv[1:])
    ~~~~^^^^^^^^^^^^^^
  File "/home/centos/workspace/psyclone/PSyclone/src/psyclone/generator.py", line 592, in main
    code_transformation_mode(input_file=args.filename,
    ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
                             recipe_file=args.script,
                             ^^^^^^^^^^^^^^^^^^^^^^^^
                             output_file=args.o,
                             ^^^^^^^^^^^^^^^^^^^
                             line_length=args.limit)
                             ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/centos/workspace/psyclone/PSyclone/src/psyclone/generator.py", line 772, in code_transformation_mode
    .psyir_from_file(input_file)
     ~~~~~~~~~~~~~~~^^^^^^^^^^^^
  File "/home/centos/workspace/psyclone/PSyclone/src/psyclone/psyir/frontend/fortran.py", line 263, in psyir_from_file
    parse_tree = self._parser(reader)
  File "/home/centos/workspace/psyclone/.venv/lib/python3.13/site-packages/fparser/two/Fortran2003.py", line 275, in __new__
    raise FortranSyntaxError(string, "")
fparser.two.utils.FortranSyntaxError: at line 177
>>>END DO

Also, I know some year ago I pushed for the removal of capturing errors in generate.py because then they were reported without the backtrace context. But I think that now that the front-end is clearly separated, I would accept surrounding the "psyir_from_file"s with a try, and just print the error without the backtrace in stderr and sys.exit(-1).

@arporter

Copy link
Copy Markdown
Member Author

Oops! Embarrassing. Fixed now.

Also, I know some year ago I pushed for the removal of capturing errors in generate.py because then they were reported without the backtrace context. But I think that now that the front-end is clearly separated, I would accept surrounding the "psyir_from_file"s with a try, and just print the error without the backtrace in stderr and sys.exit(-1).

Do you want me to do this in this PR? i.e. find all the psyir_from_file() calls in generate.py and put them inside try..excepts?

@arporter

Copy link
Copy Markdown
Member Author

The test failure was the link-checker on the GOcean web page.

@arporter

Copy link
Copy Markdown
Member Author

Ready for another look/question to answer.

@sergisiso

Copy link
Copy Markdown
Collaborator

Oops! Embarrassing. Fixed now.

Also, I know some year ago I pushed for the removal of capturing errors in generate.py because then they were reported without the backtrace context. But I think that now that the front-end is clearly separated, I would accept surrounding the "psyir_from_file"s with a try, and just print the error without the backtrace in stderr and sys.exit(-1).

Do you want me to do this in this PR? i.e. find all the psyir_from_file() calls in generate.py and put them inside try..excepts?

Yes please. But instead of discarding the backtrace as I said above, we can put the full error in the logfile. And the just the error message in stderr.

@arporter

Copy link
Copy Markdown
Member Author

I'm having a log of fun with logging. My messages seem to disappear and when I query the Logger it says that it's not enabled at the level that we initially specified. This may be because we call into fparser and it too does logging...?

@arporter

Copy link
Copy Markdown
Member Author

I've been a bit naughty but, since I've now got to grips with logging and testing, I've fixed a few of the xfails in the adjoint tests. There are still some that refer to #1235 but I thought I'd leave it as is for now.

@arporter

Copy link
Copy Markdown
Member Author

Having removed the line that effectively disabled logging, I do see a lot more log messages when running the tests (since pytest reports these by default). Hopefully there aren't any other unexpected changes but I'll fire off the integration tests to check.

@arporter

Copy link
Copy Markdown
Member Author

Integration tests have almost all finished OK (one is still waiting) so I declare this ready for review again.

@sergisiso sergisiso left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@arporter Thanks for figuring out the pytest log issues. This is getting close but the output is not what I expected, see discussion inline.

Comment thread src/psyclone/generator.py Outdated
Comment thread src/psyclone/generator.py Outdated
@arporter

Copy link
Copy Markdown
Member Author

Hopefully ready for another look now.

@sergisiso sergisiso left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @arporter , I like how this is handled now. This is approved for merging after #2967

@sergisiso

Copy link
Copy Markdown
Collaborator

I triggered the integration one last time since this touches the cli command and could affect any workflow, and everything came green, so I will proceed you merge.

@sergisiso sergisiso merged commit 6697b23 into master Jul 28, 2025
12 checks passed
@sergisiso sergisiso deleted the 3038_parse_error_failure branch July 28, 2025 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants