feat: tpu_queued_resources_startup_script/create_network/time_bound #3907
Conversation
gryczj
added
kokoro:force-run
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
samples
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
2 times, most recently
from
3276b2a
to
3ef4622
Compare
gryczj
changed the title
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
from
83378d8
to
cb97989
Compare
gryczj
changed the title
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
2 times, most recently
from
e6c1d2c
to
964cafb
Compare
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
from
964cafb
to
a66c4eb
Compare
|
Due to high costs of TPU Nodes, it was decided to use mocks in the tests. |
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
from
a66c4eb
to
9a724f9
Compare
|
@gemini review |
![code-review-assist-experimental[bot]](https://avatars.githubusercontent.com/in/919852?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| id-token: 'write' | ||
| if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail | ||
| uses: ./.github/workflows/flakybot.yaml | ||
| needs: [test] |
There was a problem hiding this comment.
Consider adding a newline at the end of the file for consistency.
| needs: [test] | |
| needs: [test] |
| // TODO(developer): Uncomment below line before running the sample. | ||
| // const {TpuClient} = require('@google-cloud/tpu').v2alpha1; |
There was a problem hiding this comment.
Since TpuClient 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): Uncomment below line before running the sample. | ||
| // tpuClient = new TpuClient(); |
There was a problem hiding this comment.
Consider instantiating the tpuClient 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 | |
| const tpuClient = new TpuClient(); |
There was a problem hiding this comment.
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.
Understood. For clarity, perhaps a comment indicating that the tpuClient 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) | |
| // const tpuClient = new TpuClient(); |
| /** | ||
| * TODO(developer): Update/uncomment these variables before running the sample. | ||
| */ |
There was a problem hiding this comment.
Update the TODO with instructions on how to set up the required environment variables or configuration.
| /** | |
| * TODO(developer): Update/uncomment these variables before running the sample. | |
| */ | |
| /** | |
| * TODO(developer): Before running the sample, | |
| * 1. Set the `GOOGLE_APPLICATION_CREDENTIALS` environment variable to the path of your service account key file. | |
| * 2. Create a network named 'compute-tpu-network' in the 'europe-west4' region. | |
| * 3. Create a subnetwork with the same name as the network in the 'europe-west4' region. | |
| */ |
There was a problem hiding this comment.
I'm not sure about this approach. This is the most common way in our repository to comment variables, that needs to be configured before running a sample.
There was a problem hiding this comment.
I understand your concern about consistency. My suggestion aimed to improve clarity and completeness for developers unfamiliar with the setup. Perhaps a compromise would be to add a brief note explaining the purpose of the environment variables and network setup before the detailed instructions, maintaining consistency with existing comments while enhancing the user experience. For example, we could preface the TODO with a sentence like: "To run this sample, you'll need to configure the following environment variables and Google Cloud resources:"
| /** | |
| * TODO(developer): Update/uncomment these variables before running the sample. | |
| */ | |
| /** | |
| * To run this sample, you'll need to configure the following environment variables and Google Cloud resources: | |
| * TODO(developer): Before running the sample, | |
| * 1. Set the `GOOGLE_APPLICATION_CREDENTIALS` environment variable to the path of your service account key file. | |
| * 2. Create a network named 'compute-tpu-network' in the 'europe-west4' region. | |
| * 3. Create a subnetwork with the same name as the network in the 'europe-west4' region. | |
| */ |
| // TODO(developer): Uncomment below line before running the sample. | ||
| // const {TpuClient} = require('@google-cloud/tpu').v2alpha1; |
There was a problem hiding this comment.
Since TpuClient 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.
Update the TODO with instructions on how to set up the required environment variables or configuration.
| /** | |
| * TODO(developer): Update/uncomment these variables before running the sample. | |
| */ | |
| /** | |
| * TODO(developer): Before running the sample, | |
| * 1. Set the `GOOGLE_APPLICATION_CREDENTIALS` environment variable to the path of your service account key file. | |
| * 2. Create a network named 'compute-tpu-network' in the 'europe-west4' region. | |
| * 3. Create a subnetwork with the same name as the network in the 'europe-west4' region. | |
| */ |
| // TODO(developer): Uncomment below line before running the sample. | ||
| // const {TpuClient} = require('@google-cloud/tpu').v2alpha1; |
There was a problem hiding this comment.
Since TpuClient 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): Uncomment below line before running the sample. | ||
| // tpuClient = new TpuClient(); |
There was a problem hiding this comment.
Consider instantiating the tpuClient 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 | |
| const tpuClient = new TpuClient(); |
| /** | ||
| * TODO(developer): Update/uncomment these variables before running the sample. | ||
| */ |
There was a problem hiding this comment.
Update the TODO with instructions on how to set up the required environment variables or configuration.
| /** | |
| * TODO(developer): Update/uncomment these variables before running the sample. | |
| */ | |
| /** | |
| * TODO(developer): Before running the sample, | |
| * 1. Set the `GOOGLE_APPLICATION_CREDENTIALS` environment variable to the path of your service account key file. | |
| * 2. Create a network named 'compute-tpu-network' in the 'europe-west4' region. | |
| * 3. Create a subnetwork with the same name as the network in the 'europe-west4' region. | |
| */ |
| @@ -0,0 +1,3 @@ | |||
| --- | |||
| env: | |||
| mocha: true | |||
There was a problem hiding this comment.
Consider adding a newline at the end of the file for consistency.
| mocha: true | |
| mocha: true |
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
from
d06385d
to
23feb0f
Compare
gryczj
force-pushed
the
tpu_queued_resources_startup_script
branch
from
23feb0f
to
7c394e1
Compare
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.