Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent segmentation fault in qfyaml with some compilers #2486

Merged

Conversation

lizziel
Copy link
Contributor

@lizziel lizziel commented Sep 27, 2024

Name and Institution (Required)

Name: Lizzie Lundgren
Institution: Harvard University

Describe the update

This PR fixes a segmentation fault in qfyaml when using certain compilers. I encounter the issue using Intel 2022.2.1 but other compilers may also trip it. The segmentation fault goes away if debug flags are turned on.

Expected changes

This is a no diff update.

Reference(s)

None

Related Github Issue

closes #1911

@lizziel lizziel added category: Bug Something isn't working topic: Runtime Error Related to runtime issues (e.g. simulation stopped w/ error) labels Sep 27, 2024
@lizziel lizziel requested a review from yantosca September 27, 2024 14:45
Copy link
Contributor

@yantosca yantosca left a comment

Choose a reason for hiding this comment

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

Thanks @lizziel. This is a simple fix. I don't know why it requires this but it might have to do with their implementation of a newer Fortran standard (which may or may not be complete).

yantosca added a commit to yantosca/qfyaml that referenced this pull request Sep 27, 2024
This implements the bug fix by @lizziel described in this PR:

geoschem/geos-chem#2486

Signed-off-by: Bob Yantosca <yantosca@seas.harvard.edu>
@yantosca
Copy link
Contributor

Thanks @lizziel for this fix! I've also updated it in the QFYAML repo: https://github.com/yantosca/qfyaml/releases/tag/0.4.4

@lizziel lizziel changed the base branch from dev/14.5.0 to dev/no-diff-to-benchmark September 27, 2024 18:17
@yantosca yantosca added this to the 14.5.1 milestone Oct 8, 2024
@yantosca yantosca added the topic: Configuration Files Related to GEOS-Chem configuration files label Oct 10, 2024
@lizziel lizziel marked this pull request as draft October 17, 2024 19:55
@lizziel
Copy link
Contributor Author

lizziel commented Oct 17, 2024

I am converting this to draft since I am getting the error on the Orion cluster again.

@lizziel lizziel force-pushed the bugfix/qfyaml_seg_fault_with_some_compilers branch from c40dae6 to 5ecc0a3 Compare October 18, 2024 14:32
@lizziel lizziel marked this pull request as ready for review October 18, 2024 16:32
@lizziel lizziel requested a review from yantosca October 18, 2024 16:33
@lizziel
Copy link
Contributor Author

lizziel commented Oct 18, 2024

I changed the update to be EXACTLY what I did on Orion, which is to a use an existing temp variable. Now it is working again.

This issue happens with Intel 2022.2.1 and possibly others.

Signed-off-by: Lizzie Lundgren <elundgren@seas.harvard.edu>
@lizziel lizziel force-pushed the bugfix/qfyaml_seg_fault_with_some_compilers branch from 5ecc0a3 to d42dc14 Compare November 14, 2024 21:03
Signed-off-by: Lizzie Lundgren <elundgren@seas.harvard.edu>
@lizziel
Copy link
Contributor Author

lizziel commented Nov 14, 2024

I rebased this onto lated dev/no-diff-to-benchmark which includes 14.5.0.

Copy link
Contributor

@yantosca yantosca left a comment

Choose a reason for hiding this comment

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

Thanks @lizziel. Good to merge!

@yantosca yantosca merged commit 7507eb5 into dev/no-diff-to-benchmark Nov 21, 2024
@yantosca yantosca deleted the bugfix/qfyaml_seg_fault_with_some_compilers branch November 21, 2024 20:27
@yantosca
Copy link
Contributor

All GEOS-Chem Classic integration tests passed, except for tagCO (known restart file issue)

==============================================================================
GEOS-Chem Classic: Execution Test Results

CodeDir   : a8aeb02 GEOS-Chem update: Merge PR #2578 (Fixes for GCHP adjoint )
GEOS-Chem : 62f3db08b Merge PR #2578 (Fixes to compile GCHP adjoint)
HEMCO     : deaa192 HEMCO 3.10.0 release
Cloud-J   : f8a2b7f Update version number for 8.0.1 release
HETP      : 2a99b24 Merge pull request #2 from geoschem/bugfix/initialize_local_variables

Using 24 OpenMP threads
Number of execution tests: 30

Submitted as SLURM job: 59103476
==============================================================================

...
gc_4x5_merra2_tagCO.................................Execute Simulation....FAIL
...
 
Summary of test results:
------------------------------------------------------------------------------
Execution tests passed: 29
Execution tests failed: 1
Execution tests not yet completed: 0

Also all tests were zero-diff w/r/t the previous integration test except for:

  • TOMAS (parallelization issue?)
  • APM (parallelization issue?)

@yantosca
Copy link
Contributor

All GCHP integration tests passed:

==============================================================================
GCHP: Execution Test Results

CodeDir       : 7c2d8e2 GEOS-Chem update: Merge PR #2578 (Fixes for GCHP adjoint )
MAPL          : 9ad63ae Merge PR #37 containing update to vertically flip imports with dimensionless pressure proxy lev coordinates
GMAO_Shared   : 4ddb3ec Merge pull request #2 from geoschem/feature/mapl-upgrade
ESMA_cmake    : ad5deba Added ecbuild as a submodule of ESMA_cmake
gFTL-shared   : 4b82492 Merge branch 'upstream_v1.5.0' into feature/v1.5.0
FMS           : 259759d Merge pull request #3 from geoschem/feature/update_gmao_libs
FVdycoreCubed : af42462 Merge PR #8 (Add PLEadv diagnostic for offline advection in GCHP)
geos-chem     : 62f3db08b Merge PR #2578 (Fixes to compile GCHP adjoint)
HEMCO         : deaa192 HEMCO 3.10.0 release
yaFyaml       : 19afe50 Merge branch 'upstream_v1.0.4' into feature/v1.0.4
pFlogger      : 2c4b724 Merge branch 'upstream_v1.9.1' into feature/v1.9.1
Cloud-J       : f8a2b7f Update version number for 8.0.1 release
HETP          : 2a99b24 Merge pull request #2 from geoschem/bugfix/initialize_local_variables

Number of execution tests: 12

Submitted as SLURM job: 59103750
==============================================================================

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%%%  All execution tests passed!  %%%
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

Also, all tests were zero-diff w/r/t the previous set of integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Bug Something isn't working topic: Configuration Files Related to GEOS-Chem configuration files topic: Runtime Error Related to runtime issues (e.g. simulation stopped w/ error)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants