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

upgrade spdlog to 1.8.5 and above #198

Merged
merged 12 commits into from
May 21, 2021
Merged

upgrade spdlog to 1.8.5 and above #198

merged 12 commits into from
May 21, 2021

Conversation

rongou
Copy link
Contributor

@rongou rongou commented Dec 11, 2020

Fallout from rapidsai/rmm#658.

@rongou rongou requested a review from a team as a code owner December 11, 2020 20:18
@rongou
Copy link
Contributor Author

rongou commented Dec 11, 2020

rerun tests

1 similar comment
@rongou
Copy link
Contributor Author

rongou commented Dec 14, 2020

rerun tests

@mike-wendt mike-wendt added 4 - Needs Reviewer Waiting for reviewer to review or respond 4 - Needs Tests Waiting for test results to make a decision labels Dec 14, 2020
mike-wendt
mike-wendt suggested changes Dec 14, 2020
View reviewed changes
conda/recipes/versions.yaml Outdated Show resolved Hide resolved
@mike-wendt mike-wendt removed the 4 - Needs Reviewer Waiting for reviewer to review or respond label Dec 14, 2020
@mike-wendt
Copy link
Contributor

Blocked until runtime images are fixed

@mike-wendt
Copy link
Contributor

rerun tests

@mike-wendt
Copy link
Contributor

@rongou there are also conda conflicts in the gpuCI/build/env-pkg-nightly tests from the previous run. I expect them to happen again so we need to find out what else uses spdlog and which version they depend on as the current pinning does not seem to work

@mike-wendt
Copy link
Contributor

rerun tests

@kkraus14
Copy link
Contributor

kkraus14 commented Dec 14, 2020 •
edited
Loading

@mike-wendt conflicts are from spdlog 1.8.2 requiring libgcc-ng >=9.3.0 and we pin 7.5.0. We may need to update our build-stack more generally as new packages are only going to be produced with this runtime version moving forward in conda-forge.

@mike-wendt mike-wendt mentioned this pull request Dec 15, 2020
Merged
1 task
@mike-wendt mike-wendt added 5 - Merge After Dependencies Depends on another PR: do not merge out of order. 4 - Waiting on Author Waiting for author to respond to review labels Dec 15, 2020
@mike-wendt
Copy link
Contributor

rerun tests

@mike-wendt
Copy link
Contributor

With the revert of #199 in #200 we'll need to create a workaround by publishing this one off pkg. Holding this PR until we can get a gcc 7.5 version published

@mike-wendt mike-wendt added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 4 - Waiting on Author Waiting for author to respond to review 5 - Merge After Dependencies Depends on another PR: do not merge out of order. labels Dec 16, 2020
mike-wendt
mike-wendt approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@mike-wendt mike-wendt left a comment

Choose a reason for hiding this comment

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

Approved, but unable to merge

@mike-wendt mike-wendt mentioned this pull request Dec 16, 2020
Merged
@rongou rongou requested a review from mike-wendt January 4, 2021 18:04
@rongou
Copy link
Contributor Author

rongou commented Jan 4, 2021

Is it ok to merge this now?

@mike-wendt
Copy link
Contributor

@kkraus14 who should take on the PR testing to get this over the line?

@kkraus14
Copy link
Contributor

kkraus14 commented Mar 4, 2021

@kkraus14 who should take on the PR testing to get this over the line?

We should probably test similarly on cuDF and BlazingSQL at the minimum before we move forward. Will try to ping folks to see if they can replicate this testing.

@rongou rongou changed the title upgrade spdlog to 1.8.2 and above upgrade spdlog to 1.8.5 and above Apr 19, 2021
@rongou rongou requested a review from kkraus14 April 22, 2021 22:46
@ajschmidt8 ajschmidt8 changed the base branch from branch-0.19 to branch-0.20 April 23, 2021 19:36
@rongou
Copy link
Contributor Author

rongou commented Apr 26, 2021

@kkraus14 can this be merged now? The RMM tests are passing rapidsai/rmm#658.

@kkraus14
Copy link
Contributor

@kkraus14 can this be merged now? The RMM tests are passing rapidsai/rmm#658.

Would still like to test cudf before merging. I'll open a PR bumping spdlog there for testing purposes.

@kkraus14 kkraus14 mentioned this pull request Apr 26, 2021
Closed
@kkraus14
Copy link
Contributor

@kkraus14 can this be merged now? The RMM tests are passing rapidsai/rmm#658.

Would still like to test cudf before merging. I'll open a PR bumping spdlog there for testing purposes.

Opened PR here to test: rapidsai/cudf#8067

@kkraus14
Copy link
Contributor

FYI -- Pinged BlazingSQL folks since they use spdlog where they're confirming that 1.8.5 is okay for them and then we'll merge.

@raydouglass
Copy link
Member

FYI -- Pinged BlazingSQL folks since they use spdlog where they're confirming that 1.8.5 is okay for them and then we'll merge.

@kkraus14 Any update on this?

@kkraus14
Copy link
Contributor

kkraus14 commented May 4, 2021

FYI -- Pinged BlazingSQL folks since they use spdlog where they're confirming that 1.8.5 is okay for them and then we'll merge.

@kkraus14 Any update on this?

PR is being thrown up to confirm this doesn't cause issues for them now. Hopefully should be straightforward.

@rongou
Copy link
Contributor Author

rongou commented May 15, 2021

@kkraus14 have you heard back from them?

@kkraus14
Copy link
Contributor

@kkraus14 have you heard back from them?

Yes, apologies, this slipped through the cracks on my end. We should be good to go to proceed here.

@rongou
Copy link
Contributor Author

rongou commented May 15, 2021

@kkraus14 can you approve this? Thanks!

@rongou
Copy link
Contributor Author

rongou commented May 17, 2021

rerun tests

1 similar comment
@rongou
Copy link
Contributor Author

rongou commented May 20, 2021

rerun tests

@ajschmidt8
Copy link
Member

@rongou, @kkraus14 just want to confirm whether or not this PR is okay to be merged. the comments seem to indicate that it is, but the DO NOT MERGE label is still applied. Should we remove that label now?

@kkraus14
Copy link
Contributor

Should be good to go, apologies.

@ajschmidt8 ajschmidt8 removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label May 21, 2021
@ajschmidt8 ajschmidt8 merged commit 63a8535 into rapidsai:branch-21.06 May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mike-wendt mike-wendt mike-wendt approved these changes

@kkraus14 kkraus14 kkraus14 approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@harrism harrism Awaiting requested review from harrism

Assignees
No one assigned
Labels
4 - Needs Tests Waiting for test results to make a decision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rongou @mike-wendt @kkraus14 @raydouglass @ajschmidt8 @harrism