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.

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

Add Zarr Flag for Pathways #180

Closed
wants to merge 1 commit into from

Conversation

SujeethJinesh
Copy link
Collaborator

Fixes / Features

  • Use Zarr flag as default for Pathways on Cloud.

Testing / Documentation

Testing details.

  • [ y/n ] Tests pass
  • [ y ] Appropriate changes to documentation are included in the PR

@@ -51,7 +51,8 @@ def get_pathways_worker_args(args) -> str:
- --xla_tpu_enable_async_collective_fusion_multiple_steps=true
- --xla_tpu_overlap_compute_collective_tc=true
- --xla_enable_async_all_gather=true
- --pathways_tmp_dir_pattern={args.pathways_gcs_location}"""
- --pathways_tmp_dir_pattern={args.pathways_gcs_location}
- --pathways_experimental_suspend_storage_mode=BACKEND_TENSORSTORE""" # This is to enable zarr for pathways. When OCDBT is tested, remove this flag

Choose a reason for hiding this comment

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

You do not need to change the PW workers here. Unless you want this flag to be consistent between the 2 jobs, you could remove this flag here.

@@ -215,7 +216,8 @@ def get_pathways_rm_args(args, system: SystemCharacteristics) -> str:
- --pathways_device_type=NONE
- --pathways_persistent_compilation_cache=false
- --pathways_tmp_dir_pattern={args.pathways_gcs_location}
- --pathways_expected_instances={expected_instances}"""
- --pathways_expected_instances={expected_instances}
- --pathways_experimental_suspend_storage_mode=BACKEND_TENSORSTORE""" # This is to enable zarr for pathways. When OCDBT is tested, remove this flag

Choose a reason for hiding this comment

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

Rename "Zarr" to "vanilla TensorStore" in the comment?

@SujeethJinesh
Copy link
Collaborator Author

Resolved with internal changes instead.

@SujeethJinesh SujeethJinesh deleted the sujinesh/add_zarr_pathways_driver branch August 26, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants