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

rough draft of quick-start/flesh out USAGE.md #207

Open
wants to merge 23 commits into
base: branch-24.06
Choose a base branch
from
Open

rough draft of quick-start/flesh out USAGE.md #207

wants to merge 23 commits into from

Conversation

msarahan
Copy link
Contributor

No description provided.

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 11, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

bdice
bdice reviewed Jan 11, 2024
View reviewed changes
USAGE.md Outdated Show resolved Hide resolved
USAGE.md Outdated Show resolved Hide resolved
USAGE.md Outdated Show resolved Hide resolved
USAGE.md Outdated Show resolved Hide resolved
@mmccarty
Copy link

What are the specific hardware and system requirements to run RAPIDS devcontainers?

mmccarty
mmccarty reviewed Jan 12, 2024
View reviewed changes
USAGE.md Outdated Show resolved Hide resolved
jrhemstad
jrhemstad reviewed Jan 12, 2024
View reviewed changes
USAGE.md Outdated Show resolved Hide resolved
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
DEVELOP.md Outdated Show resolved Hide resolved
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
mmccarty
mmccarty reviewed Jan 12, 2024
View reviewed changes
USAGE.md Outdated Show resolved Hide resolved
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
README.md Show resolved Hide resolved
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
trxcllnt
trxcllnt requested changes Jan 12, 2024
View reviewed changes
Copy link
Collaborator

@trxcllnt trxcllnt left a comment

Choose a reason for hiding this comment

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

Made some comments, but will review more later.

@msarahan
Copy link
Contributor Author

This isn't polished yet, so I'd say only review it if you really want to. I'm going to take a break and come back to it a little later with fresh eyes.

  • I split some of the content from USAGE.md into USAGE_IN_PROJECT.md. README.md summarizes what they're for, but the gist is that we have library developers at the "lowest" level, USAGE_IN_PROJECT.md. Library maintainers are people who implement devcontainers in their project. Their doc is "USAGE.md". Finally, there are people who maintain this devcontainers repo itself. Their doc is "DEVELOP.md".
  • I mostly adopted CCCL's docs regarding VS Code. I copied their images.
  • I copied @bdice's script into this PR. It is launch-devcontainer.sh - maybe it should move into scripts/

harrism
harrism reviewed Jan 18, 2024
View reviewed changes
Copy link
Member

@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.

Partial review since I won't get back to this until next week.

DEVELOP.md Outdated Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
msarahan and others added 8 commits January 18, 2024 09:27
@msarahan @harrism
Apply suggestions from code review
3048772 
Thanks for the clarifications, @harrism!

Co-authored-by: Mark Harris <[email protected]>
@msarahan @harrism
Update DEVELOP.md
8383e68 
Co-authored-by: Mark Harris <[email protected]>
@msarahan
make language less ambiguous
aaaf1ec
@msarahan
more language revision
90966c2
@trxcllnt
temporarily allow unbound variables while activating the conda env (r…
bd29430 
…apidsai#216)
@trxcllnt
Fix mambaforge shell history (backport rapidsai#219) (rapidsai#225)
635876f 
* Fix mambaforge shell history (rapidsai#219)

* disable history when restoring shell options

* Update devcontainer-feature.json

* login to dockerhub so we don't hit rate limits
@trxcllnt
Merge branch 'branch-24.04' into branch-24.02
03d3fd4
@trxcllnt
Revert "Merge branch 'branch-24.04' into branch-24.02"
b900eba 
This reverts commit 03d3fd4, reversing
changes made to 635876f.
@trxcllnt
Backport sccache v0.7.7 update to branch-24.02 (rapidsai#231)
1b85460 
* Update to sccache v0.7.7 (rapidsai#228)

* Fix `devcontainer-utils-vault-s3-init` for `[email protected]` (rapidsai#229)
harrism
harrism reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@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.

Flushing a couple more comments. I really would like this PR to become reality. It's a lot to read. I wonder if the launch-devcontainer.sh should be moved to a separate PR and merged ASAP, seems useful.

USAGE_IN_PROJECT.md Outdated Show resolved Hide resolved
USAGE_IN_PROJECT.md Outdated Show resolved Hide resolved
@msarahan msarahan marked this pull request as ready for review February 21, 2024 14:30
@msarahan
Copy link
Contributor Author

I'd like to get sign-off from at least @trxcllnt. It is a lot to read, but I think that's because it documents several use cases. I can break it up into chunks if it makes review easier.

@bdice wrote the launch script, originally called "rapids-dev" - Brad, do you have thoughts on the name here? Is it OK with you to copy the script here?

@bdice
Copy link
Contributor

bdice commented Feb 21, 2024

Brad, do you have thoughts on the name here? Is it OK with you to copy the script here?

This is fine! I'm happy with launch-devcontainer.sh.

bdice
bdice approved these changes Feb 21, 2024
View reviewed changes
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.

Generally all looks good. I support merging this after @trxcllnt has a chance to review.

USAGE_IN_PROJECT.md Outdated Show resolved Hide resolved
@trxcllnt trxcllnt changed the base branch from branch-24.02 to branch-24.04 March 8, 2024 20:21
@trxcllnt trxcllnt changed the base branch from branch-24.04 to branch-24.02 March 8, 2024 20:21
trxcllnt
trxcllnt reviewed Mar 8, 2024
View reviewed changes
DEVELOP.md Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Apr 12, 2024

This needs to be retargeted to 24.06. @msarahan After retargeting, could you respond to Paul's review and re-request another round of feedback?

@msarahan msarahan changed the base branch from branch-24.02 to branch-24.06 April 15, 2024 16:18
@msarahan
Copy link
Contributor Author

@mmccarty asked me to merge this for his use in getting oriented with devcontainers. @trxcllnt please start a new PR for any edits that you were working on, per your slack comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trxcllnt trxcllnt trxcllnt requested changes

@mmccarty mmccarty mmccarty left review comments

@harrism harrism harrism left review comments

@bdice bdice bdice approved these changes

@jameslamb jameslamb jameslamb left review comments

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@msarahan @mmccarty @bdice @trxcllnt @harrism @jameslamb @jrhemstad