Skip to content

Stop force pulling fmt in nvbench.#12768

Merged
raydouglass merged 4 commits intorapidsai:branch-23.04from
vyasr:fix/remove_nvbench_force_pull
Feb 15, 2023
Merged

Stop force pulling fmt in nvbench.#12768
raydouglass merged 4 commits intorapidsai:branch-23.04from
vyasr:fix/remove_nvbench_force_pull

Conversation

@vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 13, 2023

Description

This undoes #12064 and resolves #12065.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added 3 - Ready for Review Ready for review by team CMake CMake build issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 13, 2023
@vyasr vyasr self-assigned this Feb 13, 2023
@vyasr vyasr requested a review from a team as a code owner February 13, 2023 22:29
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 13, 2023
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

LGTM, if CI passes.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 13, 2023

I have yet to be able to reproduce these errors locally. CI shows nvbench still pulling fmt and failing to patch, the two exact things fixed in these commits, which is odd.

@robertmaynard
Copy link
Contributor

I expect that the failure to configure isn't due to nvbench patching but the above errors:

CMake Error at build/_deps/fmt-src/CMakeLists.txt:232 (add_library):
  add_library cannot create ALIAS target "fmt::fmt" because another target
  with the same name already exists.


CMake Error at build/_deps/fmt-src/CMakeLists.txt:272 (add_library):
  add_library cannot create ALIAS target "fmt::fmt-header-only" because
  another target with the same name already exists.

I expect that this is happening due to using local version of rmm that doesn't have the fmt changes. See CPM: using local package rmm@23.04.0 entry.

@bdice
Copy link
Contributor

bdice commented Feb 14, 2023

@robertmaynard I verified the local package is librmm=23.04.00a=cuda11_230213_gc9c83abb_15 from rapidsai-nightly. That's the build produced after merging rapidsai/rmm#1209. I also installed this package locally and was not able to reproduce.

@robertmaynard
Copy link
Contributor

@robertmaynard I verified the local package is librmm=23.04.00a=cuda11_230213_gc9c83abb_15 from rapidsai-nightly. That's the build produced after merging rapidsai/rmm#1209. I also installed this package locally and was not able to reproduce.

Dang. Going to look at reproducing locally to see what is going wrong

@robertmaynard
Copy link
Contributor

Pushed the changes captured by rapidsai/rapids-cmake#373 to this PR to verify it resolves the CI failures

@bdice
Copy link
Contributor

bdice commented Feb 14, 2023

Looks like CI is stuck. C++ builds have been going for 2+ hours and I don't see output.

@davidwendt
Copy link
Contributor

davidwendt commented Feb 14, 2023

Looks like CI is stuck. C++ builds have been going for 2+ hours and I don't see output.

I've been watching it too. It just seems really slow.
https://github.com/rapidsai/cudf/actions/runs/4175535853/jobs/7230651971

It looks to be about half done [546/949]
Perhaps this change required rebuilding everything? I'm not sure why it would be so slow.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 14, 2023

Yeah it doesn't look stuck, just extremely slow. I suspect that the change to the way that we use fmt is changing the compile commands in a subtle way (probably just the include path to fmt) that is invalidating all previously existing sccache entries, resulting in a recompilation of the whole library with no caching.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 15, 2023

It looks like a recent change to dask broke us, specifically dask/dask#9889. @rapidsai/cudf-dask-codeowners are there any easy solutions here? If not, I can go ahead and imply the dtypes property for cudf's DataFrameGroupBy class tomorrow, but if there's an easier patch I would prefer that given that this PR now contains multiple changes needed to unblock cudf CI.

@galipremsagar
Copy link
Contributor

galipremsagar commented Feb 15, 2023

I can go ahead and imply the dtypes property for cudf's DataFrameGroupBy class tomorrow, but if there's an easier patch I would prefer that given that this PR now contains multiple changes needed to unblock cudf CI.

That's about right, we will need .dtypes property. (I did go into dask code to see if we can change it, but looks like they are fetching .dtypes to know what columns remain after grouping is performed via a public API.)

@bdice
Copy link
Contributor

bdice commented Feb 15, 2023

I am going to propose admin-merging this to unblock libcudf CI, and @galipremsagar's PR will unblock Python CI.

@raydouglass raydouglass merged commit 12410d9 into rapidsai:branch-23.04 Feb 15, 2023
@galipremsagar galipremsagar mentioned this pull request Feb 15, 2023
3 tasks
@vyasr vyasr deleted the fix/remove_nvbench_force_pull branch February 15, 2023 18:37
@vyasr
Copy link
Contributor Author

vyasr commented Feb 15, 2023

Thanks for picking up the follow-up @galipremsagar!

rapids-bot bot pushed a commit that referenced this pull request Feb 16, 2023
This PR adds `dtypes` property to `GroupBy`, this will also fix some upstream dask breaking changes introduced in: dask/dask#9889

Issue was discovered in: #12768 (comment)

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Ashwin Srinath (https://github.com/shwina)

URL: #12783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove force download of fmt

6 participants