- Notifications
You must be signed in to change notification settings - Fork2k
feat: tpu_queued_resources_startup_script/create_network/time_bound#3907
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
snippet-botbot commentedOct 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated bysnippet-bot.
|
3276b2a to3ef4622Compare83378d8 tocb97989Comparee6c1d2c to964cafbCompare964cafb toa66c4ebComparegryczj commentedNov 12, 2024
Due to high costs of TPU Nodes, it was decided to use mocks in the tests. |
a66c4eb to9a724f9Compareiennae commentedNov 21, 2024
@gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This pull request adds support for TPU queued resources and includes creating a network, startup script, and time-bound resource. The implementation looks good, and the tests are comprehensive. Here are a few suggestions:
- Consider adding more details to the PR description about the specific functionalities added and how they address the linked issue. This will help reviewers understand the context better.
- Ensure all TODO comments are addressed before merging.
- Update the README files to reflect the new functionalities added by this PR.
.github/workflows/tpu.yaml Outdated
| id-token: 'write' | ||
| if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail | ||
| uses: ./.github/workflows/flakybot.yaml | ||
| needs: [test] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider adding a newline at the end of the file for consistency.
| needs:[test] | |
| needs:[test] |
Uh oh!
There was an error while loading.Please reload this page.
| // TODO(developer): Uncomment below line before running the sample. | ||
| // tpuClient = new TpuClient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider instantiating thetpuClient here since you're suggesting to import the class above. This makes the example runnable.
| //TODO(developer): Uncomment below line before running the sample. | |
| // tpuClient = new TpuClient(); | |
| //Instantiate a tpuClient | |
| consttpuClient=newTpuClient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is commented, becuase mocked tpuClient is used for testing and I pass mock to this function. If user wants to use real TPUClient, they will need to uncomment this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Understood. For clarity, perhaps a comment indicating that thetpuClient instantiation is for non-testing scenarios would be beneficial. This would help users understand the purpose of the commented-out code more easily.
| //TODO(developer): Uncomment below line before running the sample. | |
| // tpuClient = new TpuClient(); | |
| //Instantiate a tpuClient (uncomment for non-testing scenarios) | |
| //consttpuClient = new TpuClient(); |
Uh oh!
There was an error while loading.Please reload this page.
| // TODO(developer): Uncomment below line before running the sample. | ||
| // const {TpuClient} = require('@google-cloud/tpu').v2alpha1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SinceTpuClient is already imported in the tests, consider importing it here as well instead of relying on the parameter. This makes the code more self-contained and easier to understand.
| // TODO(developer): Uncomment below line before running the sample. | |
| //const {TpuClient} = require('@google-cloud/tpu').v2alpha1; | |
| const{TpuClient}=require('@google-cloud/tpu').v2alpha1; | |
| const{Node, NetworkConfig, QueuedResource}=require('@google-cloud/tpu').protos.google.cloud.tpu.v2alpha1; |
| /** | ||
| * TODO(developer): Update/uncomment these variables before running the sample. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Update the TODO with instructions on how to set up the required environment variables or configuration.
| /** | |
| *TODO(developer):Update/uncommentthesevariablesbeforerunningthesample. | |
| */ | |
| /** | |
| *TODO(developer):Beforerunningthesample, | |
| *1.Setthe`GOOGLE_APPLICATION_CREDENTIALS`environmentvariabletothepathofyourserviceaccountkeyfile. | |
| *2.Createanetworknamed'compute-tpu-network'inthe'europe-west4'region. | |
| *3.Createasubnetworkwiththesamenameasthenetworkinthe'europe-west4'region. | |
| */ |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tpu/test/.eslintrc Outdated
| @@ -0,0 +1,3 @@ | |||
| --- | |||
| env: | |||
| mocha: true No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider adding a newline at the end of the file for consistency.
| mocha: true | |
| mocha: true |
d06385d to23feb0fCompare1a62542 to7362b9dCompare| const networkName = 'compute-tpu-network'; | ||
| // The region of the network, that you want the node to connect to. | ||
| const region = 'europe-west4'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as in another PR, this value can be just taken from the zone value. No need to explicitly define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@m-strzelczyk fixed
7362b9d to9f48401Compare9f48401 toa9ee1f7CompareBigBlackWolf commentedDec 16, 2024
Hi@iennae, could you please take a look once again on this PR? cc:@rsamborski |
m-strzelczyk commentedJan 13, 2025
The users that are currently failing CLA had CLA signed at the time they were committing the changes. Please force-merge the PR. |
glasnt commentedMar 5, 2025
Can confirm the CLAs were signed per previous checkshttps://github.com/GoogleCloudPlatform/nodejs-docs-samples/pull/3907/checks?check_run_id=34201685809 |
glasnt commentedMar 5, 2025
@m-strzelczyk Can you please re-review? Your previous review appears to not be valid any more. |
Uh oh!
There was an error while loading.Please reload this page.
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(seeTesting)npm run lint(seeStyle)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.