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

Devcontainer GPU support #945 #946

Merged
merged 6 commits into from
Oct 25, 2022
Merged

Devcontainer GPU support #945 #946

merged 6 commits into from
Oct 25, 2022

Conversation

jacoverster
Copy link
Contributor

What does this PR do?

A small update to devcontainer.json to enable GPU support. Added "RunArgs" to devcontainer.json.

Fixes issue 945

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue? Please add a link
    to it if that's the case.
  • Did you write any new necessary tests?
  • If this adds a new model, can you run a few training steps on TPU in Colab to ensure that no XLA incompatible OP are used?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@google-cla
Copy link

google-cla bot commented Oct 19, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bhack
Copy link
Contributor

bhack commented Oct 19, 2022

You need to sign the CLA

@jacoverster
Copy link
Contributor Author

You need to sign the CLA

Hi @bhack I signed.

bhack
bhack reviewed Oct 19, 2022
View reviewed changes
.devcontainer/devcontainer.json Outdated
@@ -32,6 +32,12 @@
"version": "os-provided"
}
},

// Uncomment this if GPU support it is required
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:
probably we could remove "it" also in the previous string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I'll also update the top comment to reflect the same:

"// Uncomment this if GPU support is required"

bhack
bhack approved these changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@bhack bhack left a comment

Choose a reason for hiding this comment

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

It is ok for me.. we cannot test this in the CI cause it would require to run in cloudbuild as Gtihub Actions are CPU only.

But as it is commented by default as the other "optional" entry related to the GPU tensorflow docker image it is ok for me.

@jacoverster
Copy link
Contributor Author

It is ok for me.. we cannot test this in the CI cause it would require to run in cloudbuild as Gtihub Actions are CPU only.

But as it is commented by default as the other "optional" entry related to the GPU tensorflow docker image it is ok for me.

I also tested this locally and the container spun up properly with GPU access verified via:

import tensorflow as tf
print("Num GPUs Available: ", len(tf.config.list_physical_devices('GPU')))

@bhack
Copy link
Contributor

bhack commented Oct 19, 2022

Thanks

@bhack
Copy link
Contributor

bhack commented Oct 19, 2022

/cc @LukeWood I don't have the permission to kickstart the CI workflows, please start the CI jobs.

@bhack
Copy link
Contributor

bhack commented Oct 19, 2022

It don't know if you want to add a reference to microsoft/vscode-remote-release#3972 in the source comments.

So that we could have a reminder to imporve this when we will have a little bit of scripting support instead of requiring to the developer to comment and uncomment multiple sections just to switch from cpu to gpu and back.

@bhack bhack mentioned this pull request Oct 19, 2022
Open
@jacoverster
Copy link
Contributor Author

Good idea! I'll add a reference tomorrow morning 👍🏻 (it's evening here now)

bhack
bhack reviewed Oct 20, 2022
View reviewed changes
.devcontainer/devcontainer.json Outdated
// TODO: Improve to allow dynamic runArgs, see microsoft/vscode-remote-release#3972
// Uncomment this if GPU support is required
// "runArgs": [
// "--gpus=all"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you align this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's so strange, in my working tree it looks correct:
image

Let me try editing it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that seemed to do the trick. :)

@ianstenbit
Copy link
Contributor

/gcbrun

ianstenbit
ianstenbit approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@ianstenbit ianstenbit left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@ianstenbit ianstenbit merged commit 8c06e48 into keras-team:master Oct 25, 2022
@jacoverster
Copy link
Contributor Author

No problem, thanks for the accept and the great work you guys are doing.

@jacoverster jacoverster deleted the devcontainer_runargs branch October 25, 2022 19:16
ghost pushed a commit to y-vectorfield/keras-cv that referenced this pull request Nov 16, 2023
@jacoverster
Devcontainer GPU support keras-team#945 (keras-team#946)
c9f948b 
* update devcontainer

* update comment

* todo added to improve runargs

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

@bhack bhack bhack approved these changes

@ianstenbit ianstenbit ianstenbit approved these changes

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

Successfully merging this pull request may close these issues.

Devcontainer GPU support
3 participants
@jacoverster @bhack @ianstenbit