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

Update EAMxx docs and improve help formatters #3044

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

jgfouca
Copy link
Member

@jgfouca jgfouca commented Oct 17, 2024

No description provided.

@jgfouca jgfouca added the AT: AUTOMERGE Inform the autotester (AT) that it can merge this PR if reviewers approved, and tests pass label Oct 17, 2024
Copy link

github-actions bot commented Oct 17, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/scream/pr-preview/pr-3044/
on branch gh-pages at 2024-10-18 15:39 UTC

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SCREAM_PullRequest_Autotester_Weaver

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Build Information

Test Name: SCREAM_PullRequest_Autotester_Mappy

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Using Repos:

Repo: SCREAM (E3SM-Project/scream)
  • Branch: jgfouca/update_eamxx_testing_docs
  • SHA: 8eef1aa
  • Mode: TEST_REPO

Pull Request Author: jgfouca

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SCREAM_PullRequest_Autotester_Weaver

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Build Information

Test Name: SCREAM_PullRequest_Autotester_Mappy

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO REVIEWS HAVE BEEN PERFORMED ON THIS PULL REQUEST!

@E3SM-Autotester
Copy link
Collaborator

All Jobs Finished; status = PASSED, target_sha=890d5d7778f8882628c67cf05f406bd859c0c0b4, However Inspection must be performed before merge can occur...

Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

Looks good. I have one suggestion for the test-all docs, but I'm going to approve anyways. You decide whether to do that mod or not. I removed automerge in case you want to do it.

I like the format of good formatter.

% ./scripts/test-all-scream -h
```

If you are unsure of the cmake configuration for you development cycle, one
Copy link
Contributor

Choose a reason for hiding this comment

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

We could mention the --config-only flag, rather than ctrl-C.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also mention that they may have to run eval $(./scripts/scream-env-cmd $mach) before running make in the build folder...

Copy link
Member Author

Choose a reason for hiding this comment

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

We could mention the --config-only flag, rather than ctrl-C.

I forgot about this option. I just tried it and unfortunately you still have to wait over a minute:

real	1m20.001s

I will make note of the need to load the scream env.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, cause it has to actually run cmake, while with ctrl-C you kill it after it shows the cmake line. That makes sense.

That said, we often need to build/run just one exec. For that, we need to eventually run cmake anyways, so we should advertise the --config-only option, I think.

tests rely on pre-existing baselines. The -b option controls the baseline
location and can have the following values:
* AUTO: A common public baseline area shared by all developers
* LOCAL: A private baseline area for the current developer in the current repo
Copy link
Contributor

Choose a reason for hiding this comment

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

Side question: what do you think about removing this? I don't think we ever use this feature, and it's just more code to maintain and code path to test in the scripts... The user can simply use -b ~/temp and be done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's a nice little convenience option. I looked and it's literally 3 lines of code to support:

        local_baseline_dir = self._work_dir/"baselines"
        if self._baseline_dir == "LOCAL":
            self._baseline_dir = local_baseline_dir

bartgol
bartgol previously approved these changes Oct 18, 2024
@bartgol bartgol removed the AT: AUTOMERGE Inform the autotester (AT) that it can merge this PR if reviewers approved, and tests pass label Oct 18, 2024
@jgfouca jgfouca added the AT: AUTOMERGE Inform the autotester (AT) that it can merge this PR if reviewers approved, and tests pass label Oct 18, 2024
@jgfouca jgfouca requested a review from bartgol October 18, 2024 15:38
@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SCREAM_PullRequest_Autotester_Weaver

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Build Information

Test Name: SCREAM_PullRequest_Autotester_Mappy

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Using Repos:

Repo: SCREAM (E3SM-Project/scream)
  • Branch: jgfouca/update_eamxx_testing_docs
  • SHA: 29fdc0d
  • Mode: TEST_REPO

Pull Request Author: jgfouca

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SCREAM_PullRequest_Autotester_Weaver

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Build Information

Test Name: SCREAM_PullRequest_Autotester_Mappy

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
THE LAST COMMIT TO THIS PULL REQUEST HAS NOT BEEN REVIEWED YET!

@E3SM-Autotester
Copy link
Collaborator

All Jobs Finished; status = PASSED, target_sha=624e53c81ea2d950f8520e33a209e14cbba0fec1, However Inspection must be performed before merge can occur...

Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

Looks good. I would maybe plan for another iteration, where we cover more power-user things, like --config-only, or the -l flag in combination with stuff in ~/.cime, to allow development on your own laptop/workstation.

But this is covering the basics, so we should merge.

@E3SM-Autotester
Copy link
Collaborator

The base branch has been updated since the last successful testing.

  • last PASS base branch sha: 624e53c
  • current base branch sha : 0fb8150
    The AutoTester will discard the last PASS, and re-test the PR from scratch

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SCREAM_PullRequest_Autotester_Weaver

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Build Information

Test Name: SCREAM_PullRequest_Autotester_Mappy

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Using Repos:

Repo: SCREAM (E3SM-Project/scream)
  • Branch: jgfouca/update_eamxx_testing_docs
  • SHA: 29fdc0d
  • Mode: TEST_REPO

Pull Request Author: jgfouca

@E3SM-Autotester
Copy link
Collaborator

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SCREAM_PullRequest_Autotester_Weaver

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

Build Information

Test Name: SCREAM_PullRequest_Autotester_Mappy

  • Build Num: -1
  • Status: SKIPPED

Jenkins Parameters

Parameter Name Value

@E3SM-Autotester E3SM-Autotester merged commit d782bf0 into master Oct 18, 2024
7 checks passed
@E3SM-Autotester E3SM-Autotester removed the AT: AUTOMERGE Inform the autotester (AT) that it can merge this PR if reviewers approved, and tests pass label Oct 18, 2024
@E3SM-Autotester E3SM-Autotester deleted the jgfouca/update_eamxx_testing_docs branch October 18, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants