-
Notifications
You must be signed in to change notification settings - Fork 113
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
Comments
cc @kiukchung @d4l3k |
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 |
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. |
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 |
|
❓ 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:
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.
The text was updated successfully, but these errors were encountered: