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

Make GAAS work #231

Merged
merged 10 commits into from
Jan 4, 2023
Merged

Make GAAS work #231

merged 10 commits into from
Jan 4, 2023

Conversation

bena-nasa
Copy link
Collaborator

So, this is the PR to get GAAS working after it was changed to use ExtData which was apparently never tested and relied on a non-existent feature. I have finally implemented this feature that is addressed in this companion PR in MAPL:
GEOS-ESM/MAPL#1797
In the PR here in chemistry I have updated GAAS as well as the OPS version of the ExtData2G yaml file for GAAS

@bena-nasa bena-nasa added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix This fixes a bug labels Nov 29, 2022
@bena-nasa bena-nasa requested review from a team as code owners November 29, 2022 19:40
@mathomp4 mathomp4 added the Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Nov 29, 2022
@mathomp4
Copy link
Member

I'm going to block this with a label for now until we can get MAPL 2.32 with @bena-nasa's PR in.

@mathomp4
Copy link
Member

mathomp4 commented Dec 9, 2022

MAPL 2.33 is now in a PR in GEOSgcm: GEOS-ESM/GEOSgcm#497

When @sdrabenh pulls that into GEOSgcm main, then this PR should be able to pass CI (once CI is rerun)

@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Contingent - DNA, 0 diff, bugfix

@mmanyin
Copy link
Contributor

mmanyin commented Jan 3, 2023

@mathomp4 OK if we remove the DNA label?

@mathomp4
Copy link
Member

mathomp4 commented Jan 3, 2023

@mathomp4 OK if we remove the DNA label?

I think so. But do you mind if I make an update to the changelog to note "Now requires MAPL 2.32 or higher"? → I updated the changelog.

@mathomp4
Copy link
Member

mathomp4 commented Jan 3, 2023

Actually, I think this is a good reason to make this a full on minor release (1.11.0) because it requires MAPL 2.32 to BUILD as @bena-nasa had to make a new procedure in MAPL.

@tclune would a minor release cover this? I don't think a major is needed since it's not like chemistry itself changed, but rather it's just a build issue in a library.

@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Contingent - DNA, 0 diff, bugfix

@mmanyin
Copy link
Contributor

mmanyin commented Jan 3, 2023

@mathomp4 I'm fine with a minor release.

@tclune
Copy link
Collaborator

tclune commented Jan 3, 2023

@tclune would a minor release cover this? I don't think a major is needed since it's not like chemistry itself changed, but rather it's just a build issue in a library.

Yes. Packages that use GAAS do not need to change. So it is a minor release for GAAS. And just to be pedantic ... GAAS did not change because MAPL changed. Rather the desired change exploited a new feature from MAPL.
L. So no backward compatibility is violated.

Prepare for release on 1/4/23
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: Contingent - DNA, 0 diff, bugfix

@mmanyin mmanyin removed the Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Jan 4, 2023
@mathomp4
Copy link
Member

mathomp4 commented Jan 4, 2023

I think this is good then! And I see @mmanyin removed the label

Copy link
Contributor

@mmanyin mmanyin left a comment

Choose a reason for hiding this comment

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

The PR is zero-diff for standard runs. Trust in Ben that GAAS now operates properly.

@mmanyin mmanyin merged commit 243772f into develop Jan 4, 2023
@mmanyin mmanyin deleted the feature/bmauer/make_gaas_work_again branch January 4, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix This fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants