flexflow / FlexFlow

FlexFlow Serve: Low-Latency, High-Performance LLM Serving
https://flexflow.readthedocs.io
Apache License 2.0
1.67k stars 223 forks source link

Code Coverage Support #1380

Closed Bob-Chen222 closed 4 months ago

Bob-Chen222 commented 5 months ago

Description of changes:

Hi @lockshaw, this is the PR for code coverage I added code coverage support in cmake lists My next step would be about integrating the output codecoverage into reviewable

Related Issues:

Linked Issues:

Issues closed by this PR:


This change is Reviewable

Bob-Chen222 commented 4 months ago

Reviewed 3 of 3 files at r1, all commit messages. Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Bob-Chen222)

CMakeLists.txt line 90 at r1 (raw file):

include(nccl)
include(CodeCoverage)
append_coverage_compiler_flags()

Does this mean we're always building with coverage instrumentation?

flake.nix line 100 at r1 (raw file):

              tl-expected
              lcov # for code coverage
              xdg_utils # for xdg-open to open html files

Use the version from the system instead. If xdg-open isn't present then proj can just shout at the user. It makes more sense to have the external system configure it since the whole point is to allow the user to configure which browser to open on their system

Yes, it is currently always built with code coverage. I can add an optional flag for the build command so that we can choose whether to include code coverage. Do you think this would work?

lockshaw commented 4 months ago

@Bob-Chen222 Let's implement it in a separate PR--I'd like to get this merged.

It would be best for it to be bundled into a command, like proj test --coverage or something like that so users don't have to explicitly think about how stuff is built, etc. We can even have two build directories to avoid rebuilds. Can you create an issue and we can discuss it further there?

Also, reviewable is preferred over github comments where possible :slightly_smiling_face: