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

Reorganization of the GHA files #2801

Merged
merged 138 commits into from
Nov 18, 2023
Merged

Conversation

TotoGaz
Copy link
Contributor

@TotoGaz TotoGaz commented Nov 3, 2023

The integrated tests can be run as part of the workflow when the label ci: run integrated tests is associated to the PR.

A bundle of the new reference files and of the logs is sent to the cloud and made available for download to the developer. (For the sake of merging, this step deactivated in this PR.).

For the moment, integrated tests are only requested to be run on 2 procs. This can be changed easily.

Important note: while the integrated tests can be run, they are not passing:

    Status     : TestCase                                      : Directory                                                         : Elapsed : Resources : TestStep  
    ---------- : --------------------------------------------- : ----------------------------------------------------------------- : ------- : --------- : ----------
    FAIL RUN   : Sneddon_embeddedFrac_benchmark_01             : embeddedFractures/Sneddon_embeddedFrac_benchmark_01               : 0:00:03 : 0:00:03   : geos      
    ---------- : --------------------------------------------- : ----------------------------------------------------------------- : ------- : --------- : ----------
    FAIL CHECK : PoroElastic_permeableFault_smoke_01           : poroElasticCoupling/PoroElastic_permeableFault_smoke_01           : 0:00:04 : 0:00:04   : restartcheck
    FAIL CHECK : PoroElastic_impermeableFault_smoke_01         : poroElasticCoupling/PoroElastic_impermeableFault_smoke_01         : 0:00:04 : 0:00:04   : restartcheck
    FAIL CHECK : grav_seg_c1ppu_hyst_01                        : compositionalMultiphaseFlow/grav_seg_c1ppu_hyst_01                : 0:00:05 : 0:00:05   : restartcheck
    FAIL CHECK : ThermoPoroElastic_consolidation_sequential_02 : thermoPoromechanics/ThermoPoroElastic_consolidation_sequential_02 : 0:00:16 : 0:00:32   : restartcheck
    FAIL CHECK : grav_seg_c1ppu_hyst_02                        : compositionalMultiphaseFlow/grav_seg_c1ppu_hyst_02                : 0:00:03 : 0:00:06   : restartcheck
    ---------- : --------------------------------------------- : ----------------------------------------------------------------- : ------- : --------- : ----------

Also:

  • The GHA yaml files now only manage input data and job dependencies.
    In the end, the job informations are later provided to ci_build_and_test_in_container.sh directly.
    Script ci_build_and_test.sh, which was previously calling ci_build_and_test_in_container.sh could be removed.
  • The number of environment variables in the GHA workflow is reduced to nearly the maximum.
    (LA variables remain because I was afraid of conflicts. This can be done in a follow-up PR if wanted.)
    The scripts now use getopt instead.
  • No duplicated call to the scripts for all the builds. One does all the cases with an decent number of if.
  • Initial job is_pull_request_a_draft provides all the output requested from the PR (assignees, draft, ...).
    It uses a single rest call and uses the GHA output pattern.
  • No more final job to find if everything went OK; "gate" jobs allow to enter conditional jobs instead of using if in yaml which make the job optional (?).
  • The GEOS_TPL_TAG is not defined in .github/workflows/ci_tests.yml anymore. It's defined in and read from .devcontainer/devcontainer.json to avoid duplication. Documentation is updated accordingly.
  • Extracting the data (geos executable bundle or integrated tests results) was simplified and do not require docker cp anymore.
    The bundling process does not need to have access to out-of-scope information (like the commit or the PR number).
  • The credentials for sccache is simplified a bit: no need for a specific mount point and copy of secrets.
  • codespaces can now write in the GEOS-DEV submodules of the GEOS repository (in particular integratedTests).
  • Fixes brittle assumptions when building the ats run scripts (in https://github.com/GEOS-DEV/integratedTests/pull/58)

Relates to issue #2800

@TotoGaz TotoGaz self-assigned this Nov 3, 2023
@TotoGaz TotoGaz changed the title Running the integrated tests in the CI Reorganization of the GHA files Nov 14, 2023
@TotoGaz TotoGaz added flag: requires updated submodule(s) and removed type: testing Unit tests, non-regression testing, ... labels Nov 14, 2023
Copy link
Collaborator

@CusiniM CusiniM left a comment

Choose a reason for hiding this comment

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

@bmhan12, @rrsettgast if you are fine with this I would just go ahead and merge it.

@TotoGaz TotoGaz added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Nov 17, 2023
Copy link
Contributor

@bmhan12 bmhan12 left a comment

Choose a reason for hiding this comment

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

No more final job to find if everything went OK; "gate" jobs allow to enter conditional jobs instead of using if in yaml which make the job optional (?).

Awesome! I just want to make sure: in the case where CUDA jobs are not run ('ci: ready to be merged' is not set), does the workflow show an expected failure status?

I found that without the convoluted "check_that_all_jobs_succeeded" job, a workflow will pass and the PR is mergeable without having ran the CUDA tests.

@TotoGaz
Copy link
Contributor Author

TotoGaz commented Nov 17, 2023

Awesome! I just want to make sure: in the case where CUDA jobs are not run ('ci: ready to be merged' is not set), does the workflow show an expected failure status?

I found that without the convoluted "check_that_all_jobs_succeeded" job, a workflow will pass and the PR is mergeable without having ran the CUDA tests.

Hi @bmhan12 and thanks for the review.
Yes, the workflow is tagged as "failed" if any of the are_..._requested jobs do not find their corresponding label. I've been checking that especially.
I think that if we use the if: in the yaml, then the jobs is not done but considered optional...
I didn't find a way to fix this using GHA's yaml feature
Maybe it's possible, in the yaml, to do some if not and exit 1, but that duplicate the condition...

@TotoGaz TotoGaz removed the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Nov 18, 2023
@TotoGaz TotoGaz added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Nov 18, 2023
@TotoGaz TotoGaz merged commit 4821322 into develop Nov 18, 2023
@TotoGaz TotoGaz deleted the feature/TotoGaz/runIntegratedTests branch November 18, 2023 15:55
@TotoGaz
Copy link
Contributor Author

TotoGaz commented Nov 18, 2023

Hi @bmhan12 I've just merged the PR. I did notice that the check_that_all_jobs_succeeded was required and I was not able to modify this (most likely it's in some GitHub admin pane I can't access). So I've put it back for the sake of merging. We can manage this in a follow-up PR where we'll manage the integrated tests as well.

ouassimkh pushed a commit that referenced this pull request Feb 16, 2024
The integrated tests will be run as part of the workflow when the label "ci: run integrated tests" is associated to the PR.
A bundle of the new reference files and of the logs is sent to the cloud and made available for download to the developer. (For the sake of merging, this step deactivated in this PR.).

For the moment, integrated tests are only requested to be run on 2 procs. This can be changed easily.

Important note: while the integrated tests can be run, they are not passing yet. This is why the integrated tests feature is deactivated. We'll manage this in a follow-up PR.

Also:

-The GHA yaml files now only manage input data and job dependencies. In the end, the job informations are later provided to ci_build_and_test_in_container.sh directlyScript ci_build_and_test.sh, which was previously calling ci_build_and_test_in_container.sh could be removed.
-The number of environment variables in the GHA workflow is reduced to nearly the maximum. (LA variables remain because I was afraid of conflicts. This can be done in a follow-up PR if wanted.) The scripts now use getopt instead.
- No duplicated call to the scripts for all the builds. One does all the cases with an decent number of if.
-Initial job is_pull_request_a_draft provides all the output requested from the PR (assignees, draft, ...). It uses a single rest call and uses the GHA output pattern.
- No more final job to find if everything went OK; "gate" jobs allow to enter conditional jobs instead of using if in yaml which make the job optional (?).
- The GEOS_TPL_TAG is not defined in .github/workflows/ci_tests.yml anymore. It's defined in and read from .devcontainer/devcontainer.json to avoid duplication. Documentation is updated accordingly.
- Extracting the data (geos executable bundle or integrated tests results) was simplified and do not require docker cp anymore. The bundling process does not need to have access to out-of-scope information (like the commit or the PR number).
- The credentials for sccache is simplified a bit: no need for a specific mount point and copy of secrets. codespaces can now write in the GEOS-DEV submodules of the GEOS repository (in particular integratedTests).
- Fixes brittle assumptions when building the ats run scripts (in Fix paths when creating ats run scripts 
 ntegratedTests#58)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: requires updated submodule(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants