feat: tpu_queued_resources_startup_script/create_network/time_bound #3907

gryczj · 2024-10-23T11:57:54Z

Description

Fixes #

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

snippet-bot · 2024-10-23T11:58:03Z

Here is the summary of changes.

You are about to add 3 region tags.

tpu/queuedResources/createQueuedResourceNetwork.js:20, tag tpu_queued_resources_network
tpu/queuedResources/createQueuedResourceStartupScript.js:20, tag tpu_queued_resources_startup_script
tpu/queuedResources/createQueuedResourceTimeBound.js:20, tag tpu_queued_resources_time_bound

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

gryczj · 2024-11-12T17:08:16Z

Due to high costs of TPU Nodes, it was decided to use mocks in the tests.

iennae · 2024-11-21T06:41:14Z

@gemini review

code-review-assist-experimental

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.

code-review-assist-experimental · 2024-11-21T06:57:24Z

.github/workflows/tpu.yaml

+      id-token: 'write'
+    if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail
+    uses: ./.github/workflows/flakybot.yaml
+    needs: [test]


Consider adding a newline at the end of the file for consistency.

Suggested change

needs: [test]

needs: [test]

tpu/queuedResources/createQueuedResourceNetwork.js

code-review-assist-experimental · 2024-11-21T06:57:25Z

tpu/queuedResources/createQueuedResourceNetwork.js

+  // TODO(developer): Uncomment below line before running the sample.
+  // tpuClient = new TpuClient();


Consider instantiating the tpuClient here since you're suggesting to import the class above. This makes the example runnable.

Suggested change

// TODO(developer): Uncomment below line before running the sample.

// tpuClient = new TpuClient();

// Instantiate a tpuClient

const tpuClient = new TpuClient();

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

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.

Suggested change

// TODO(developer): Uncomment below line before running the sample.

// tpuClient = new TpuClient();

// Instantiate a tpuClient (uncomment for non-testing scenarios)

// const tpuClient = new TpuClient();

tpu/queuedResources/createQueuedResourceNetwork.js

code-review-assist-experimental · 2024-11-21T06:57:26Z

tpu/queuedResources/createQueuedResourceStartupScript.js

+  // TODO(developer): Uncomment below line before running the sample.
+  // const {TpuClient} = require('@google-cloud/tpu').v2alpha1;


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.

Suggested change

// 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;

code-review-assist-experimental · 2024-11-21T06:57:26Z

tpu/queuedResources/createQueuedResourceStartupScript.js

+  /**
+   * TODO(developer): Update/uncomment these variables before running the sample.
+   */


Update the TODO with instructions on how to set up the required environment variables or configuration.

Suggested change

/**

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

*/

tpu/queuedResources/createQueuedResourceTimeBound.js

code-review-assist-experimental · 2024-11-21T06:57:27Z

tpu/test/.eslintrc

@@ -0,0 +1,3 @@
+---
+env:
+  mocha: true


Consider adding a newline at the end of the file for consistency.

Suggested change

mocha: true

mocha: true

m-strzelczyk · 2024-12-03T13:05:00Z

tpu/queuedResources/createQueuedResourceNetwork.js

+  const networkName = 'compute-tpu-network';
+
+  // The region of the network, that you want the node to connect to.
+  const region = 'europe-west4';


Same as in another PR, this value can be just taken from the zone value. No need to explicitly define.

@m-strzelczyk fixed

BigBlackWolf · 2024-12-16T10:50:22Z

Hi @iennae, could you please take a look once again on this PR?

cc: @rsamborski

gryczj added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Oct 23, 2024

gryczj requested review from a team as code owners October 23, 2024 11:57

product-auto-label bot added samples Issues that are directly related to samples. api: workflows Issues related to the Workflows API. labels Oct 23, 2024

gryczj added the api: tpu Issues related to the Cloud TPU API. label Oct 23, 2024

gryczj force-pushed the tpu_queued_resources_startup_script branch 2 times, most recently from 3276b2a to 3ef4622 Compare October 23, 2024 18:12

gryczj changed the title ~~feat: tpu_queued_resources_startup_script~~ feat: tpu_queued_resources_startup_script/create_network Oct 24, 2024

gryczj force-pushed the tpu_queued_resources_startup_script branch from 83378d8 to cb97989 Compare October 25, 2024 09:57

gryczj changed the title ~~feat: tpu_queued_resources_startup_script/create_network~~ feat: tpu_queued_resources_startup_script/create_network/time_bound Oct 25, 2024

gryczj force-pushed the tpu_queued_resources_startup_script branch 2 times, most recently from e6c1d2c to 964cafb Compare October 30, 2024 11:48

TetyanaYahodska mentioned this pull request Oct 30, 2024

feat(tpu): add tpu queued resources time bound sample. GoogleCloudPlatform/java-docs-samples#9617

Open

11 tasks

gryczj force-pushed the tpu_queued_resources_startup_script branch from 964cafb to a66c4eb Compare November 12, 2024 17:05

gryczj force-pushed the tpu_queued_resources_startup_script branch from a66c4eb to 9a724f9 Compare November 20, 2024 12:30

code-review-assist-experimental bot reviewed Nov 21, 2024

View reviewed changes

gryczj force-pushed the tpu_queued_resources_startup_script branch from d06385d to 23feb0f Compare November 26, 2024 09:08

gryczj assigned iennae and unassigned iennae Nov 28, 2024

gryczj requested a review from iennae November 28, 2024 09:08

gryczj force-pushed the tpu_queued_resources_startup_script branch 3 times, most recently from 1a62542 to 7362b9d Compare December 3, 2024 08:27

gryczj requested a review from a team as a code owner December 3, 2024 08:27

m-strzelczyk approved these changes Dec 3, 2024

View reviewed changes

gryczj added 3 commits December 3, 2024 17:50

feat: tpu_queued_resources_startup_script

78c9912

feat: tpu_queued_resources_create_network

50cc2e8

feat: tpu_queued_resources_time_bound

c992903

gryczj force-pushed the tpu_queued_resources_startup_script branch from 7362b9d to 9f48401 Compare December 3, 2024 16:53

gryczj requested a review from m-strzelczyk December 3, 2024 16:53

Use mocked TPUClient in tests

a9ee1f7

gryczj force-pushed the tpu_queued_resources_startup_script branch from 9f48401 to a9ee1f7 Compare December 4, 2024 13:57

BigBlackWolf self-requested a review December 5, 2024 11:54

BigBlackWolf added 2 commits December 9, 2024 17:53

correct zone definition

b8b5fa5

correct the region network in test

954e4ec

Merge branch 'main' into tpu_queued_resources_startup_script

5fc0b13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tpu_queued_resources_startup_script/create_network/time_bound #3907

feat: tpu_queued_resources_startup_script/create_network/time_bound #3907

gryczj commented Oct 23, 2024 •

edited

Loading

snippet-bot bot commented Oct 23, 2024 •

edited

Loading

gryczj commented Nov 12, 2024

iennae commented Nov 21, 2024

code-review-assist-experimental bot left a comment

code-review-assist-experimental bot Nov 21, 2024

code-review-assist-experimental bot Nov 21, 2024

gryczj Nov 21, 2024

code-review-assist-experimental bot Nov 21, 2024

code-review-assist-experimental bot Nov 21, 2024

code-review-assist-experimental bot Nov 21, 2024

code-review-assist-experimental bot Nov 21, 2024

m-strzelczyk Dec 3, 2024

gryczj Dec 3, 2024

BigBlackWolf commented Dec 16, 2024

		// TODO(developer): Uncomment below line before running the sample.
		// tpuClient = new TpuClient();

		// TODO(developer): Uncomment below line before running the sample.
		// const {TpuClient} = require('@google-cloud/tpu').v2alpha1;

-  /**
-   * 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.
+   */

feat: tpu_queued_resources_startup_script/create_network/time_bound #3907

Are you sure you want to change the base?

feat: tpu_queued_resources_startup_script/create_network/time_bound #3907

Conversation

gryczj commented Oct 23, 2024 • edited Loading

Description

Checklist

snippet-bot bot commented Oct 23, 2024 • edited Loading

gryczj commented Nov 12, 2024

iennae commented Nov 21, 2024

code-review-assist-experimental bot left a comment

Choose a reason for hiding this comment

code-review-assist-experimental bot Nov 21, 2024

Choose a reason for hiding this comment

code-review-assist-experimental bot Nov 21, 2024

Choose a reason for hiding this comment

gryczj Nov 21, 2024

Choose a reason for hiding this comment

code-review-assist-experimental bot Nov 21, 2024

Choose a reason for hiding this comment

code-review-assist-experimental bot Nov 21, 2024

Choose a reason for hiding this comment

code-review-assist-experimental bot Nov 21, 2024

Choose a reason for hiding this comment

code-review-assist-experimental bot Nov 21, 2024

Choose a reason for hiding this comment

m-strzelczyk Dec 3, 2024

Choose a reason for hiding this comment

gryczj Dec 3, 2024

Choose a reason for hiding this comment

BigBlackWolf commented Dec 16, 2024

gryczj commented Oct 23, 2024 •

edited

Loading

snippet-bot bot commented Oct 23, 2024 •

edited

Loading