Skip to content

Comments

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

Open
msarahan wants to merge 23 commits intorapidsai:branch-24.06from
msarahan:quickstart
Open

rough draft of quick-start/flesh out USAGE.md#207
msarahan wants to merge 23 commits intorapidsai:branch-24.06from
msarahan:quickstart

Conversation

@msarahan
Copy link
Contributor

@msarahan msarahan commented Jan 11, 2024

No description provided.

@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
@mmccarty
Copy link

mmccarty commented Jan 12, 2024

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

mmccarty
mmccarty reviewed Jan 12, 2024
View reviewed changes
jrhemstad
jrhemstad reviewed Jan 12, 2024
View reviewed changes
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
mmccarty
mmccarty reviewed Jan 12, 2024
View reviewed changes
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
trxcllnt
trxcllnt reviewed Jan 12, 2024
View reviewed changes
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 and others added 2 commits January 12, 2024 12:15
@msarahan @trxcllnt
Apply suggestions from code review
ad07fbb 
Co-authored-by: Paul Taylor <178183+trxcllnt@users.noreply.github.com>
@msarahan
flesh out 3-doc readme, copy img from CCCL
bda365a
@msarahan
Copy link
Contributor Author

msarahan commented Jan 16, 2024

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.

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 <783069+harrism@users.noreply.github.com>
@msarahan @harrism
Update DEVELOP.md
8383e68 
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
@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 `sccache@v0.7.7` (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.

@msarahan @harrism
Apply suggestions from code review
de5bd94 
Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>
@msarahan msarahan marked this pull request as ready for review February 21, 2024 14:30
@msarahan
Copy link
Contributor Author

msarahan commented Feb 21, 2024

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.

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

msarahan commented Apr 22, 2024

@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

@bdice bdice bdice approved these changes

@jameslamb jameslamb jameslamb left review comments

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

+1 more reviewer

@harrism harrism harrism left review comments

Reviewers whose approvals may not affect merge requirements

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