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.

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

Update the README #291

Merged
merged 30 commits into from
Sep 19, 2023
Merged

Update the README #291

merged 30 commits into from
Sep 19, 2023

Conversation

jrhemstad
Copy link
Collaborator

@jrhemstad jrhemstad commented Jul 31, 2023

Description

closes #290 #66

Updates the main README.md to provide a comprehensive landing page for the new monorepo.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jrhemstad jrhemstad marked this pull request as ready for review September 5, 2023 21:53
@jrhemstad jrhemstad requested a review from a team as a code owner September 5, 2023 21:53
@jrhemstad jrhemstad requested review from elstehle and removed request for a team September 5, 2023 21:53
@jrhemstad
Copy link
Collaborator Author

I'd appreciate some fresh eyes on this since it's a lot of new content and I want to make sure I'm covering all the things users care about.

Paging @bdice @kkraus14 @vyasr @leofang @cliffburdick

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Reviewed up through CTK compatibility section. Overall looks great. Just some copy editing. If you agree to accept the eradication of "we", then suggest doing so through the whole doc, not just my suggestions.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Some partial review

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

Comments attached. Overall this is good information and I appreciate all the hard work that has gone into defining these rules. I agree with Mark's suggestions, especially on things like avoiding "we."

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
### C++ Dialects
- C++11 (Deprecated in Thrust/CUB, to be removed in next major version)
- C++14 (Deprecated in Thrust/CUB, to be removed in next major version)
- C++17
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace. Please trim the trailing whitespace in this file, there are other places where this is happening.

Suggested change
- C++17
- C++17

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

We encourage our users to adhere to the following guidelines in order to minimize the risk of disruptions from accidentally depending on parts of CCCL that we do not consider part of the public API:

- Do not add any declarations to the `thrust::`, `cub::`, `nv::`, or `cuda::` namespaces unless an exception is noted for a specific symbol, e.g., specializing a type trait.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the first time that nv:: is mentioned? Is there context needed for that? (I don't actually know what's in that namespace.)

Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Reviewed up to the API section.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
This means you cannot bring an older version of CCCL to a newer CTK.
In other words, **CCCL is never forwards compatible with the CUDA Toolkit.**

The table below summarizes compatibility of the CTK and CCCL:
Copy link
Member

Choose a reason for hiding this comment

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

Q: Do we have a table somewhere showing the version map from existing CTK 11.x, 12.x to CCCL versions? If so, perhaps we can link it here as a reference?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@harrism harrism left a comment

Choose a reason for hiding this comment

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

A couple more suggestions, but nothing blocking, so I'm approving. Great work!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@leofang
Copy link
Member

leofang commented Sep 14, 2023

LGTM so far!

@jrhemstad jrhemstad merged commit acf83c1 into NVIDIA:main Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEA]: Update the README [EPIC] Document support and compatibility guarantees
7 participants