Code coverage intel analysis #298

abhaasgoyal · 2024-07-05T00:40:58Z

Resolves #91

Description

Add CLI option - benchcab gen_codecov for running code coverage analysis for all runs grouped by each realisation's Intel build. Runs only if codecov: true in config.yaml. Added as a workflow step in fluxsite runs.
Add new config.yaml keyword (codecov) - which provides the compiler build flags to run gen_codecov later on

Note: As of now, codecov analysis is only supported w.r.t. fluxsite tasks

Testing

On the following config.yaml file, run benchcab:

realisations:
  - repo:
      git:
        branch: main
    patch:
      cable:
        check:
         ranges: 1

fluxsite:
 experiment: AU-Tum

codecov: true

modules: [
  intel-compiler/2021.1.1,
  netcdf/4.7.4,
  openmpi/4.1.0
]

$ benchcab run -v

After the job has completed, run the code coverage utility:

$ benchcab codecov

On inspection, it generates the files as expected at runs/codecov/R0/.

codecov · 2024-07-05T00:44:54Z

Codecov Report

Attention: Patch coverage is 58.00000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 70.21%. Comparing base (bb0ab3d) to head (1dcd270).
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/benchcab/benchcab.py	10.00%	18 Missing ⚠️
src/benchcab/model.py	55.55%	16 Missing ⚠️
src/benchcab/coverage.py	78.94%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #298      +/-   ##
==========================================
+ Coverage   69.97%   70.21%   +0.23%     
==========================================
  Files          18       20       +2     
  Lines         986     1128     +142     
==========================================
+ Hits          690      792     +102     
- Misses        296      336      +40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ccarouge

I only had a quick look but there is something in the design I don't like (if I got it right).

I don't think we need both the "build_option" entry in the config.yaml and the codecov command.

I can't think of a test case to run benchcab tests with a code compiled with Debug option (the runs are too long to do with Debug). So the build_option entry is only here to ensure we can compile with code coverage on when running the coverage command. But setting build_option and the coverage command are independent of each other.

So what happens if I forget to set build_option before running the coverage command? Shouldn't the coverage command force building with the code coverage options instead of being dependent on an independent entry in the config file?

Actually, I'm not sure how the codecov command is to be used. Are we supposed to have run benchcab first (benchcab run) with code coverage options on for the compilation and then run benchcab codecov?
Are you assuming we aren't going to check the code coverage manually?

abhaasgoyal · 2024-07-09T06:29:03Z

I can't think of a test case to run benchcab tests with a code compiled with Debug option

Oh I see, so I thought we wouldn't have to rebuild benchcab again, but if the tests take too long, we can leave debug as an option (we still need debug-codecov right?). Although I was thinking we could have additional options in the future, like release-profile, which benchmarks the code in Release builds and needs different flags.

But setting build_option and the coverage command are independent of each other.

benchcab coverage is run after the fluxsite jobs are completed (similar to benchcab fluxsite-bitwise-cmp), however build_option is set in config.yaml to pass in the flags before the build (which has additional binary instrumentation), before the tests are run. So, there is a dependency of benchcab coverage being run only if build_option has coverage.

Are we supposed to have run benchcab first (benchcab run) with code coverage options on for the compilation and then run benchcab codecov?

Yes, after the jobs are completed

Are you assuming we aren't going to check the code coverage manually?

I thought benchcab codecov could make things simpler by providing a central utility function to run commands across all realisations. Now that I think about it, we can have designs like:

Manual running of benchcab codecov: Provide an error if config.yaml doesn't have build_option set as debug-codecov
Automated running of benchcab codecov: After the PBS job if config.yaml has debug-codecov, unless skip flag has it.

Both options above make the relationship explicit.

Happy to hear alternative designs/requirements.

SeanBryan51

Thanks for implementing the core functionality Abhaas.

I think adding a benchcab codecov subcommand is how I would approach the code coverage generation step as well.

I think I agree with Claire that specifying build_option isn't required for code coverage and is a bit outside the scope of the PR. I also recommend trying to keep changes minimal in the Model.build method as that functionality will eventually be replaced with spack.

Instead of enabling code coverage via build_option, have you considered introducing a global codecov option? This could either be a configuration file option (e.g. codecov: true) or a command line option for the relevant subcommands (e.g. benchcab run --codecov where --codecov is propagated to other commands like build, fluxsite-submit-job, etc).

src/benchcab/internal.py

SeanBryan51 · 2024-07-10T00:25:36Z

src/benchcab/model.py

+            if compiler_id != "ifort":
+                msg = f"""For code coverage, the only supported compiler is `ifort`
+                User has {compiler_id} in their environment"""
+                raise RuntimeError(msg)


Is this necessary? The compiler will throw an error if it cannot recognise the given compiler flags. This would also exit with an error when trying to run code coverage using Intels ifx compiler which isn't desired.

Yes, that is true, although it was decided that it would be clearer if benchcab gave the error message before passing in to the compiler (I can also add check for ifx). @ccarouge thoughts?

src/benchcab/model.py

SeanBryan51 · 2024-07-10T00:47:19Z

src/benchcab/model.py

+        with self.modules_handler.load([internal.DEFAULT_MODULES["cmake"], *modules]):
+
+            # $FC is loaded after compiler module is loaded,
+            # but we need runs/ dir relative to project rootdir
+            env_fc = os.environ.get("FC", "")
+            self.logger.debug(
+                f"Getting environment variable for compiler $FC = {env_fc}"
            )
+            build_flags = self.get_build_flags(mpi, build_option, env_fc)
+            env_fc = None
+
+            with chdir(path_to_repo):


It would be nice if we didn't have to split the with statement in two. If you remove the inspection of the FC environment variable you should be able to query the CMake arguments outside the with statement entirely.

Yes, will make the with statement in 1 line if compiler_id isn't required.

src/benchcab/coverage.py

src/benchcab/benchcab.py

abhaasgoyal · 2024-07-10T02:00:01Z

Instead of enabling code coverage via build_option, have you considered introducing a global codecov option? This could either be a configuration file option (e.g. codecov: true) or a command line option for the relevant subcommands (e.g. benchcab run --codecov where --codecov is propagated to other commands like build, fluxsite-submit-job, etc).

I think it would be better to have codecov: true at the top level, since if we run benchcab gen_codecov at the end, we can check if config.yaml had already set it.

SeanBryan51

I made a few minor suggestions but overall I'm happy with it. Approving

src/benchcab/cli.py

src/benchcab/model.py

tests/test_cli.py

tests/test_model.py

src/benchcab/utils/pbs.py

src/benchcab/model.py

src/benchcab/coverage.py

abhaasgoyal · 2024-07-11T04:26:32Z

I see (implemented the respective changes) and tested. I'll squash the commits once ready to be merged.

SeanBryan51

LGTM

ccarouge

Good to go. Just a minor change if you want to add it before merging.

src/benchcab/data/test/pbs_jobscript_skip_bitwise.sh

abhaasgoyal force-pushed the 91-code-coverage-analysis branch 3 times, most recently from ef27b5b to 1b543fd Compare July 9, 2024 05:11

abhaasgoyal marked this pull request as ready for review July 9, 2024 05:15

abhaasgoyal requested review from a team and SeanBryan51 and removed request for a team July 9, 2024 05:19

abhaasgoyal self-assigned this Jul 9, 2024

ccarouge requested changes Jul 9, 2024

View reviewed changes

SeanBryan51 requested changes Jul 10, 2024

View reviewed changes

src/benchcab/benchcab.py Outdated Show resolved Hide resolved

abhaasgoyal requested review from ccarouge and SeanBryan51 July 10, 2024 03:06

SeanBryan51 approved these changes Jul 11, 2024

View reviewed changes

src/benchcab/cli.py Outdated Show resolved Hide resolved

src/benchcab/model.py Show resolved Hide resolved

tests/test_cli.py Outdated Show resolved Hide resolved

tests/test_model.py Outdated Show resolved Hide resolved

SeanBryan51 requested changes Jul 11, 2024

View reviewed changes

src/benchcab/utils/pbs.py Outdated Show resolved Hide resolved

abhaasgoyal requested a review from SeanBryan51 July 11, 2024 03:18

SeanBryan51 requested changes Jul 11, 2024

View reviewed changes

src/benchcab/model.py Outdated Show resolved Hide resolved

src/benchcab/coverage.py Show resolved Hide resolved

abhaasgoyal added the priority:high High priority issues that should be included in the next release. label Jul 11, 2024

abhaasgoyal requested a review from SeanBryan51 July 11, 2024 04:28

SeanBryan51 approved these changes Jul 11, 2024

View reviewed changes

abhaasgoyal removed the request for review from ccarouge July 11, 2024 04:37

abhaasgoyal force-pushed the 91-code-coverage-analysis branch from 679729b to f4c3c19 Compare July 11, 2024 04:40

abhaasgoyal requested a review from ccarouge July 11, 2024 04:44

ccarouge approved these changes Jul 12, 2024

View reviewed changes

src/benchcab/data/test/pbs_jobscript_skip_bitwise.sh Outdated Show resolved Hide resolved

Support for Intel code-coverage analysis (#91)

1dcd270

abhaasgoyal force-pushed the 91-code-coverage-analysis branch from b65c2df to 1dcd270 Compare July 15, 2024 01:29

abhaasgoyal merged commit ab1d22d into main Jul 15, 2024
2 of 3 checks passed

abhaasgoyal deleted the 91-code-coverage-analysis branch July 15, 2024 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code coverage intel analysis #298

Code coverage intel analysis #298

abhaasgoyal commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

ccarouge left a comment

abhaasgoyal commented Jul 9, 2024 •

edited

Loading

SeanBryan51 left a comment

SeanBryan51 Jul 10, 2024

abhaasgoyal Jul 10, 2024

SeanBryan51 Jul 10, 2024

abhaasgoyal Jul 10, 2024

abhaasgoyal commented Jul 10, 2024

SeanBryan51 left a comment

abhaasgoyal commented Jul 11, 2024

SeanBryan51 left a comment

ccarouge left a comment

Code coverage intel analysis #298

Code coverage intel analysis #298

Conversation

abhaasgoyal commented Jul 5, 2024 • edited Loading

Description

Testing

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

ccarouge left a comment

Choose a reason for hiding this comment

abhaasgoyal commented Jul 9, 2024 • edited Loading

SeanBryan51 left a comment

Choose a reason for hiding this comment

SeanBryan51 Jul 10, 2024

Choose a reason for hiding this comment

abhaasgoyal Jul 10, 2024

Choose a reason for hiding this comment

SeanBryan51 Jul 10, 2024

Choose a reason for hiding this comment

abhaasgoyal Jul 10, 2024

Choose a reason for hiding this comment

abhaasgoyal commented Jul 10, 2024

SeanBryan51 left a comment

Choose a reason for hiding this comment

abhaasgoyal commented Jul 11, 2024

SeanBryan51 left a comment

Choose a reason for hiding this comment

ccarouge left a comment

Choose a reason for hiding this comment

abhaasgoyal commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

abhaasgoyal commented Jul 9, 2024 •

edited

Loading