Skip to content

Conversation

kiukchung
Copy link
Contributor

Summary:
Since we now allow each Role to specify its own workspace through the Role.workspace attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the caching_build_workspace_and_update_role() method to the WorkspaceMixIn. This is the preferred method (over build_workspace_and_update_role()) that subclasses of WorkspaceMixIn should implement.

Until we can completely deprecate build_workspace_and_update_role(), we bridge the two by having the default implementation of caching_build_workspace_and_update_role() call build_workspace_and_update_role().

To support caching_build_workspace_and_update_role(), this diff also does the following:

  1. Renames build_workspace_and_udpate_role2() to caching_build_workspace_and_update_role().

  2. Iterating over the roles and building the workspace for each role is now pushed into the WorkspaceMixIn.build_workspace_and_update_roles() instead of it being in the runner.

  3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the Workspace class's merge_into() method.

Reviewed By: AbishekS

Differential Revision: D84466900

Copy link

meta-codesync bot commented Oct 16, 2025

@kiukchung has exported this pull request. If you are a Meta employee, you can view the originating Diff in D84466900.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 16, 2025
facebook-github-bot pushed a commit that referenced this pull request Oct 16, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
@facebook-github-bot facebook-github-bot force-pushed the export-D84466900 branch 2 times, most recently from d7e7413 to 3e4daf8 Compare October 16, 2025 23:07
facebook-github-bot pushed a commit that referenced this pull request Oct 16, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
kiukchung added a commit that referenced this pull request Oct 17, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
facebook-github-bot pushed a commit that referenced this pull request Oct 17, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
kiukchung added a commit that referenced this pull request Oct 17, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
kiukchung added a commit that referenced this pull request Oct 17, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2025

Codecov Report

❌ Patch coverage is 84.52381% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.56%. Comparing base (426b91a) to head (ef852bd).

Files with missing lines Patch % Lines
torchx/test/fixtures.py 83.33% 7 Missing ⚠️
torchx/specs/api.py 69.23% 4 Missing ⚠️
torchx/workspace/api.py 90.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
- Coverage   91.69%   91.56%   -0.13%     
==========================================
  Files          83       83              
  Lines        6539     6593      +54     
==========================================
+ Hits         5996     6037      +41     
- Misses        543      556      +13     
Flag Coverage Δ
unittests 91.56% <84.52%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

facebook-github-bot pushed a commit that referenced this pull request Oct 17, 2025
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
…een roles (#1149)

Summary:

Since we now allow each `Role` to specify its own workspace through the `Role.workspace` attribute, there are cases where all the roles have the same workspace and base image. In this case, we don't have to re-build the workspace image for each role since the ephemerals will all have the same content!

This diff introduces the `caching_build_workspace_and_update_role()` method to the `WorkspaceMixIn`. This is the preferred method (over `build_workspace_and_update_role()`) that subclasses of `WorkspaceMixIn` should implement.

Until we can completely deprecate `build_workspace_and_update_role()`, we bridge the two by having the default implementation of `caching_build_workspace_and_update_role()` call `build_workspace_and_update_role()`.

To support `caching_build_workspace_and_update_role()`, this diff also does the following:

1. Renames `build_workspace_and_udpate_role2()` to `caching_build_workspace_and_update_role()`.

2. Iterating over the roles and building the workspace for each role is now pushed into the `WorkspaceMixIn.build_workspace_and_update_roles()` instead of it being in the runner.

3. Pushes the logic of merging of multi-directory workspace into a single tmp dir to the `Workspace` class's `merge_into()` method.

Reviewed By: AbishekS

Differential Revision: D84466900
@meta-codesync meta-codesync bot merged commit 5053c87 into main Oct 17, 2025
19 of 22 checks passed
@meta-codesync meta-codesync bot deleted the export-D84466900 branch October 17, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants