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

Workspace patch is applied only on role[0] image #782

Open
kurman opened this issue Oct 22, 2023 · 5 comments
Open

Workspace patch is applied only on role[0] image #782

kurman opened this issue Oct 22, 2023 · 5 comments
Labels
enhancement New feature or request question User questions

Comments

@kurman
Copy link
Contributor

kurman commented Oct 22, 2023

❓ Questions and Help

Per https://github.com/pytorch/torchx/blob/main/torchx/runner/api.py#L362-L370, we assume that patch needs to be applied only for a single role. Effectively assumes that:

  1. role0 is the only image that needs to be updated
  2. workspace is mapped to image of role0.

This issue has surfaced for an internal Meta user.

Question

Should we treat this as a bug an apply patch to all the roles or introduce proper mapping between workspaces and roles? This hasn't surfaced since most of our customers use single role, but looks like it is broken. My personal preference is to provide warning to users when multiple roles are defined first, then add non-default option to specify workspace for each role name.

@kurman
Copy link
Contributor Author

kurman commented Oct 22, 2023

cc @kiukchung @d4l3k

@d4l3k
Copy link
Contributor

d4l3k commented Oct 22, 2023

This behavior is intentional so I wouldn't consider this a bug. If we have different images for different roles, patching the same changes into both images doesn't make sense in many cases

Are they using the same image for multiple roles? If so we could add a special case to reuse the same patch for both since it's the same image

@kurman
Copy link
Contributor Author

kurman commented Oct 23, 2023

Based on the initial feedback looks like using the same image for multiple roles. If we go this route, this has to be treated a special case. One way we can approach this is to check if all the roles have the same image, then apply the patch.

The right thing I believe is to have a workspace:role mapping, but would like to see more users that will require this feature.

@d4l3k
Copy link
Contributor

d4l3k commented Oct 23, 2023

Adding in some logic to apply the patch to all roles that share an image seems very reasonable

@kurman is this something that can be applied to all workspaces or will this only be applied to the internal Meta workspace?

Having workspace:role mapping adds a lot of complexity to both the code and how users interact with torchx so would prefer to wait until we get more feedback about this

@d4l3k d4l3k added enhancement New feature or request question User questions labels Oct 23, 2023
@kurman
Copy link
Contributor Author

kurman commented Oct 23, 2023

@kurman is this something that can be applied to all workspaces or will this only be applied to the internal Meta workspace?

build_workspace_and_update_role currently operates on a single role and I won't be able to localize the changes per workspace type if we are to keep the contract the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question User questions
Projects
None yet
Development

No branches or pull requests

2 participants