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.

Sign up for GitHub

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

Jump to bottom

update fmt and spdlog across RAPIDS #56

Open
jameslamb opened this issue Apr 24, 2024 · 4 comments
Open

update fmt and spdlog across RAPIDS #56

jameslamb opened this issue Apr 24, 2024 · 4 comments
Assignees
@jameslamb
Milestone
v24.10

Comments

@jameslamb
Copy link
Member

jameslamb commented Apr 24, 2024 •
edited

Description

RAPIDS projects that need fmt and spdlog use rapids-cmake to find it, via functions like rapids_cpm_fmt() (docs link).

rapids-cmake is currently carrying patches to both of those libraries, and as a result always downloads sources of them. (see rapidsai/rapids-cmake#525 for details).

Those patches have since been upstreamed and made it into official releases of those projects (in their main source control and on conda-forge). This proposes upgrading to those new versions across RAPIDS and dropping the patches in rapids-cmake.

Benefits of this work

  • reduces RAPIDS package sizes a bit
  • fixes warnings about file-clobbering in conda builds across many RAPIDS projects
  • removes some patches from rapids-cmake and therefore the need to reason about them in future upgrades

Acceptance Criteria

  • rapids-cmake is pulling in fmt >= 10.2.1
  • rapids-cmake is pulling in spdlog >= 1.13
  • all RAPIDS projects enforce those floors in their build environments
  • there are 0 warnings about clobbering fmt and spdlog in rmm's conda build CI logs
  • 0 RAPIDS projects' CI is failing as a result of these changes

Approach

On a branch, modify rapids-cmake/cpm/versions.json (code link) such that:

  • the versions for fmt and spdlog are bumped to these new ones
  • the patches for those libraries are removed

Follow "Overriding RAPIDS CMake" (rapids-cmake docs) to point builds of at least rmm, cudf, and raft at your branch of rapids-cmake with these changes. Also modify those projects' dependencies.yaml and/or conda recipe meta.yaml to change their version constraints on fmt and spdlog.

NOTE: this is a great task for rapids-reviser (https://github.com/rapidsai/rapids-reviser/tree/main/examples/fmt-10-spdlog-1.12).

Confirm that CI succeeds and that the correct versions of fmt and spdlog are being pulled in.

For rmm, re-use this PR for that purpose: rapidsai/rmm#1508. If this change also results in there being 0 remaining conda clobbering warnings in rmm, leave in the conda config --set path_conflict prevent in that PR's build scripts, so we'll be alerted by a CI failure if something around this changes in the future.

Notes

What makes you confident that these patches can be removed?

From #54 (comment)

fmt's most recent release was 10.2.1, so that patch we're carrying around for the 10.1.1 version could be dropped.

The spdlog patch we've been carrying around has been upstreamed

What are "clobber warnings" and how does this help with them?

As described in #54 and rapidsai/rmm#1508, conda builds of rmm and other RAPIDS packages downstream of it are emitting dozens of warnings like these:

This transaction has incompatible packages due to a shared path.
  packages: conda-forge/linux-aarch64::fmt-10.2.1-h2a328a1_0, file:///tmp/conda-bld-output/linux-aarch64::librmm-24.06.00a16-cuda12_240419_g9dfd9070_16
  path: 'include/fmt/chrono.h'

This transaction has incompatible packages due to a shared path.
   packages: conda-forge/linux-aarch64::spdlog-1.12.0-h6b8df57_2, file:///tmp/conda-bld-output/linux-aarch64::librmm-24.06.00a16-cuda12_240419_g9dfd9070_16
   path: 'include/spdlog/async.h'

These are saying that the librmm conda package contains files whose paths exactly conflict with those from the fmt and spdlog packages from conda-forge. When such packages are installed together, one will overwrite the other, which could lead to runtime issues.

Removing the patches in rapids-cmake makes it more likely that builds will find the files they need from the fmt and spdlog conda-forge packages already present in the build environment, and therefore not vendor them, and therefore not ship a copy that causes conflicts.
@jameslamb jameslamb self-assigned this Apr 24, 2024
This was referenced Apr 24, 2024
Closed
Closed
Open
Closed
Closed
Closed
Closed
@jameslamb
Copy link
Member Author

@bdice drew my attention to these global pins in conda-forge-pinning-feestock:

fmt:
  - '10'

spdlog:
  - '1.12'

(code link)

I think the fmt one shouldn't be a problem for this migration, but the spdlog one will. There's an ongoing conda-forge migration to spdlog==1.13.0, but I don't know how to estimate how long that'll take: https://conda-forge.org/status/migration/spdlog113.

I think we should NOT update any RAPIDS packages to depending on the spdlog==1.13.0 conda-forge package until that conda-forge migration to that version is done. Does that seem right @jakirkham @bdice ?

References:

@bdice
Copy link
Contributor

bdice commented Apr 30, 2024 •
edited

Yes, we should try to keep RAPIDS in sync with conda-forge. For these, I would move them forward once the migrator passes some critical threshold (for spdlog, I might base it on when packages like mamba and micromamba are migrated).

For the purpose of fixing our conda clobbering issues, we may want to work on some kind of vendoring solution (centralized via rapids-core-dependencies or copied in each RAPIDS conda package) because we have to carry patches for fmt and spdlog in rapids-cmake anyway. If we do that, we can remove all host/run dependencies on the conda-forge packages of fmt and spdlog and instead get them from upstream via rapids-cmake / CPM.

Also, if we vendor these and remove the dependencies from the RAPIDS conda packages, it would also make it possible to decouple from the global conda-forge pinnings and manage version updates solely through rapids-cmake (the status quo requires changes in both rapids-cmake and conda recipes).

@jameslamb jameslamb mentioned this issue May 1, 2024
Merged
7 tasks
rapids-bot bot pushed a commit to rapidsai/rapids-cmake that referenced this issue May 1, 2024
@jameslamb
limit pinning tests to CPM-downloaded projects (#599)
bc1c006 
Modifies pinning tests from #530:
* to only test projects that were downloaded by CPM (e.g. ignoring the `fmt` that might already exist in the build environment)
* to echo out `pinned_versions.json` and `versions.json` in logs from failed tests, to make debugging faster

## Notes for Reviewers

#592 proposes some testing changes that aren't specific to the goals of that PR.

Since that PR might be stuck for a bit (rapidsai/build-planning#56 (comment)), this proposes pulling those out into a separate PR:

* so that other changes in this project benefit from them
* to shrink the diff of #592 and therefore the risk of merge conflicts

### How I tested this

Pushed a commit with the new test error message content changes but keeping `fmt` in the failing tests, to confirm that the expected tests failed.

<details><summary>got the expected outputs (click me)</summary>

```text
The following tests FAILED:
	698 - cpm_generate_pins-nested-makefile (Failed)
	700 - cpm_generate_pins-nested-ninja (Failed)
	702 - cpm_generate_pins-nested-ninja_multi-config (Failed)
	722 - cpm_generate_pins-simple-makefile (Failed)
	724 - cpm_generate_pins-simple-ninja (Failed)
	726 - cpm_generate_pins-simple-ninja_multi-config (Failed)
```

And they failed in the expected way more informative logs!

```text
CMake Error at CMakeLists.txt:51 (message):
  pinned fmt tag (10.2.1) should differ compared to baseline 10.2.1

  pinned_versions.json:

  {

    "always_download" : true,
    "git_shallow" : false,
    "git_tag" : "${version}",
    "git_url" : "https://github.com/fmtlib/fmt.git",
    "version" : "10.2.1"

  }

  versions.json:

  {

    "git_tag" : "${version}",
    "git_url" : "https://github.com/fmtlib/fmt.git",
    "version" : "10.2.1"

  }
```

([build link](https://github.com/rapidsai/rapids-cmake/actions/runs/8837613213/job/24267079292?pr=592))

</details>

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

URL: #599
This was referenced Jul 18, 2024
Open
Closed
Closed
Closed
Closed
Closed
@harrism harrism mentioned this issue Jul 22, 2024
Open
@kkraus14
Copy link

FYI -- There's active migrations for spdlog 1.14 and fmt 11 going on in conda-forge right now:

I believe RAPIDS is now multiple versions behind on spdlog which will make solving environments painful.

@bdice
Copy link
Contributor

bdice commented Aug 14, 2024

Thanks for the heads-up @kkraus14! We'll discuss and try to land an update for fmt/spdlog in the next RAPIDS release.

@sisodia1701 sisodia1701 added this to the RAPIDS v24.10 milestone Aug 14, 2024
@sisodia1701 sisodia1701 modified the milestones: RAPIDS v24.10, v24.10 Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jameslamb jameslamb

Labels
None yet
Projects
None yet
Milestone
v24.10
Development

No branches or pull requests
4 participants
@kkraus14 @bdice @jameslamb @sisodia1701