CMake jit_preprocess_files function only runs when needed #7872

robertmaynard · 2021-04-06T15:03:03Z

This correct a couple of issues with the jit_preprocess_files function.

It now always makes sure that the output directory that the jit headers are going into will exist before executing. So if a user deletes <build_dir>/include everything will work as expected
The output of the jitify tool are files that end in .jit.hpp not .jit. This is required to allow generators to build the minimal incremental compilation graph
Mark the input source files as a dependency so when that file is modified, the build-system knows to recompile the jit file.

cwharris · 2021-04-06T15:09:23Z

Since ARG_OUTPUT is used as both the explicit OUTPUT and to build a list of dependencies, this change doesn't effect the dependency tree, because custom commands are run every build, yes?

ttnghia · 2021-04-06T15:28:05Z

I applied this PR but that still does not help---I still get the same error:

cpp/src/binaryop/binaryop.cpp:23:10: fatal error: jit_preprocessed_files/binaryop/jit/kernel.cu.jit.hpp: No such file or directory
   23 | #include <jit_preprocessed_files/binaryop/jit/kernel.cu.jit.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

robertmaynard · 2021-04-06T15:31:57Z

I applied this PR but that still does not help---I still get the same error:

cpp/src/binaryop/binaryop.cpp:23:10: fatal error: jit_preprocessed_files/binaryop/jit/kernel.cu.jit.hpp: No such file or directory
   23 | #include <jit_preprocessed_files/binaryop/jit/kernel.cu.jit.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

If you run make jitify_preprocess_run ( or ninja jitify_preprocess_run ) do you see that the file <build_dir>/include/ jit_preprocessed_files/binaryop/jit/kernel.cu.jit.hpp exists?

ttnghia · 2021-04-06T15:35:02Z

I just check that. The folder include/jit_preprocessed_files is empty---there is not any subfolder. Nothing there.

ttnghia · 2021-04-06T15:37:57Z

FYI, I tested this PR by deleting everything and checked out the latest branch-0.20, then applied this PR and did a fresh build.

robertmaynard · 2021-04-06T15:40:24Z

Since ARG_OUTPUT is used as both the explicit OUTPUT and to build a list of dependencies, this change doesn't effect the dependency tree, because custom commands are run every build, yes?

Hmmm. I think you are correct as is they will be recompiled each time. But we can fix that to only re-compile when the input file is modified.

robertmaynard · 2021-04-06T15:41:07Z

I can reproduce this behavior on Ubuntu 20.04 with the Makefile Generator but ninja Generator correctly generated files.

ttnghia · 2021-04-06T15:44:59Z

I can reproduce this behavior on Ubuntu 20.04 with the Makefile Generator but ninja generator correctly generated.

Sorry that I mistakenly modified your post---I intentionally wanted to reply-quote instead. My head is a bit fuzzy in the meantime.

My system is exactly like that: Ubuntu 20.04 with the Makefile Generator.

PS: It is funny that github allows users to modify non-authored posts. (oh, that is funny [I promise not to edit any more of your posts] - @cwharris)

ttnghia · 2021-04-06T15:53:48Z

Your new commits fix the issue. Yeah.

robertmaynard · 2021-04-06T15:56:51Z

@cwharris

This now fixes the issue where builds with the Unix Makefiles generator not creating the output directory. It also fixes a couple of issues allowing for better incremental builds. Now CMake will only re-run the JIT step when the input files change. Previously it always was running the JIT step as the output files listed in OUTPUT didn't exist ( either bad names or due to the above generator problem ).

cwharris · 2021-04-06T16:01:13Z

Thanks @robertmaynard !

I didn't see this locally because ninja was generating the files whereas cmake does not.

cpp/cmake/Modules/JitifyPreprocessKernels.cmake

kkraus14 · 2021-04-06T16:29:57Z

@gpucibot merge

kkraus14 · 2021-04-06T18:26:34Z

rerun tests

ttnghia · 2021-04-06T20:26:27Z

Rerun tests.

kkraus14 · 2021-04-07T00:28:59Z

FYI -- ops team is building a new rapids-build-env package that has updated conda packages. The previous package was built at a perfect timing in between the new fsspec and gcsfs versions being released that left things in a broken state.

kkraus14 · 2021-04-07T03:45:24Z

rerun tests

codecov · 2021-04-07T05:59:41Z

Codecov Report

Merging #7872 (3ebe3da) into branch-0.19 (7871e7a) will increase coverage by 0.88%.
The diff coverage is n/a.

❗ Current head 3ebe3da differs from pull request most recent head 1d416df. Consider uploading reports for the commit 1d416df to get more accurate results

@@               Coverage Diff               @@
##           branch-0.19    #7872      +/-   ##
===============================================
+ Coverage        81.86%   82.74%   +0.88%     
===============================================
  Files              101      103       +2     
  Lines            16884    17701     +817     
===============================================
+ Hits             13822    14647     +825     
+ Misses            3062     3054       -8

Impacted Files	Coverage Δ
python/cudf/cudf/utils/dtypes.py	`83.44% <0.00%> (-6.08%)`	⬇️
python/cudf/cudf/core/column/lists.py	`87.41% <0.00%> (-3.99%)`	⬇️
python/cudf/cudf/core/column/decimal.py	`92.92% <0.00%> (-1.95%)`	⬇️
python/cudf/cudf/core/groupby/groupby.py	`92.41% <0.00%> (-1.04%)`	⬇️
python/cudf/cudf/utils/utils.py	`85.36% <0.00%> (-0.07%)`	⬇️
python/dask_cudf/dask_cudf/backends.py	`89.58% <0.00%> (-0.05%)`	⬇️
python/cudf/cudf/__init__.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/io/feather.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/core/__init__.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/utils/ioutils.py	`78.71% <0.00%> (ø)`
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8c00f1...1d416df. Read the comment docs.

kkraus14 · 2021-04-07T12:48:14Z

@gpucibot merge

) This correct a couple of issues with the `jit_preprocess_files` function. 1. It now always makes sure that the output directory that the jit headers are going into will exist before executing. So if a user deletes `<build_dir>/include` everything will work as expected 2. The output of the jitify tool are files that end in `.jit.hpp` not `.jit`. This is required to allow generators to build the minimal incremental compilation graph 3. Mark the input source files as a dependency so when that file is modified, the build-system knows to recompile the jit file. Authors: - Robert Maynard (https://github.com/robertmaynard) Approvers: - Christopher Harris (https://github.com/cwharris) - Nghia Truong (https://github.com/ttnghia) - Jason Lowe (https://github.com/jlowe) - Keith Kraus (https://github.com/kkraus14) URL: rapidsai#7872

jit_preprocess_files properly specifies the correct output paths

a6e2a25

robertmaynard requested a review from a team as a code owner April 6, 2021 15:03

github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Apr 6, 2021

cwharris approved these changes Apr 6, 2021

View reviewed changes

robertmaynard added 2 commits April 6, 2021 11:46

jitify now properly tracks input files

84da36f

jit_preprocess_files make sure output directory always exists

1b0711e

robertmaynard changed the title ~~jit_preprocess_files properly specifies the correct output paths~~ CMake jit_preprocess_files function only runs when needed Apr 6, 2021

ttnghia approved these changes Apr 6, 2021

View reviewed changes

cwharris added the 3 - Ready for Review Ready for review by team label Apr 6, 2021

jlowe approved these changes Apr 6, 2021

View reviewed changes

jlowe added bug Something isn't working non-breaking Non-breaking change labels Apr 6, 2021

trxcllnt reviewed Apr 6, 2021

View reviewed changes

cpp/cmake/Modules/JitifyPreprocessKernels.cmake Outdated Show resolved Hide resolved

trxcllnt reviewed Apr 6, 2021

View reviewed changes

cpp/cmake/Modules/JitifyPreprocessKernels.cmake Outdated Show resolved Hide resolved

Cleanup JitifyPreprocessKernels.cmake as requested in code review

1d416df

kkraus14 approved these changes Apr 6, 2021

View reviewed changes

kkraus14 removed the 3 - Ready for Review Ready for review by team label Apr 6, 2021

kkraus14 added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Apr 6, 2021

rapids-bot bot merged commit 5f84ea8 into rapidsai:branch-0.19 Apr 7, 2021

robertmaynard deleted the fix/jit_preprocess_files_dependency_tracking branch April 7, 2021 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake jit_preprocess_files function only runs when needed #7872

CMake jit_preprocess_files function only runs when needed #7872

robertmaynard commented Apr 6, 2021 •

edited

Loading

cwharris commented Apr 6, 2021

ttnghia commented Apr 6, 2021

robertmaynard commented Apr 6, 2021

ttnghia commented Apr 6, 2021 •

edited

Loading

ttnghia commented Apr 6, 2021 •

edited

Loading

robertmaynard commented Apr 6, 2021

robertmaynard commented Apr 6, 2021 •

edited by ttnghia

Loading

ttnghia commented Apr 6, 2021 •

edited

Loading

ttnghia commented Apr 6, 2021

robertmaynard commented Apr 6, 2021

cwharris commented Apr 6, 2021

kkraus14 commented Apr 6, 2021

kkraus14 commented Apr 6, 2021

ttnghia commented Apr 6, 2021

kkraus14 commented Apr 7, 2021 •

edited

Loading

kkraus14 commented Apr 7, 2021

codecov bot commented Apr 7, 2021 •

edited

Loading

kkraus14 commented Apr 7, 2021

CMake jit_preprocess_files function only runs when needed #7872

CMake jit_preprocess_files function only runs when needed #7872

Conversation

robertmaynard commented Apr 6, 2021 • edited Loading

cwharris commented Apr 6, 2021

ttnghia commented Apr 6, 2021

robertmaynard commented Apr 6, 2021

ttnghia commented Apr 6, 2021 • edited Loading

ttnghia commented Apr 6, 2021 • edited Loading

robertmaynard commented Apr 6, 2021

robertmaynard commented Apr 6, 2021 • edited by ttnghia Loading

ttnghia commented Apr 6, 2021 • edited Loading

ttnghia commented Apr 6, 2021

robertmaynard commented Apr 6, 2021

cwharris commented Apr 6, 2021

kkraus14 commented Apr 6, 2021

kkraus14 commented Apr 6, 2021

ttnghia commented Apr 6, 2021

kkraus14 commented Apr 7, 2021 • edited Loading

kkraus14 commented Apr 7, 2021

codecov bot commented Apr 7, 2021 • edited Loading

Codecov Report

kkraus14 commented Apr 7, 2021

robertmaynard commented Apr 6, 2021 •

edited

Loading

ttnghia commented Apr 6, 2021 •

edited

Loading

ttnghia commented Apr 6, 2021 •

edited

Loading

robertmaynard commented Apr 6, 2021 •

edited by ttnghia

Loading

ttnghia commented Apr 6, 2021 •

edited

Loading

kkraus14 commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading