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

[torchx][api] Deprecate Torchx Session Name #227

Closed
aivanou opened this issue Oct 7, 2021 · 3 comments
Closed

[torchx][api] Deprecate Torchx Session Name #227

aivanou opened this issue Oct 7, 2021 · 3 comments
Assignees
Labels
enhancement New feature or request module: runner issues related to the torchx.runner and torchx.scheduler modules RFC Request for Feedback & Roadmaps
Milestone

Comments

@aivanou
Copy link
Contributor

aivanou commented Oct 7, 2021

Torchx Runner has a concept of session_name. When user creates a Runner, he/she may provide session_name. If no argument provided, the session_name will be set to: torchx_$user .

This parameter is then used to create a full job name as: session_name-$app_der.name .

The problem is that the default session name torchx_$user is not bound by the max length, and in theory can be very long. This may affect user jobs without them understanding the root cause.

In order to solve this, we can make default session name as torchx_ instead, and omit $user part.

@d4l3k d4l3k added enhancement New feature or request module: runner issues related to the torchx.runner and torchx.scheduler modules labels Oct 7, 2021
@d4l3k
Copy link
Member

d4l3k commented Oct 7, 2021

It's useful in certain contexts I think. With schedulers where the name is provided in metadata it's not useful but for things like Volcano jobs having that can be beneficial since there's not an easy way to view metadata via vcctl.

However, I don't think that really needs to be on the runner level and could be decided on the scheduler specific level. We don't include the session name or user name currently in the Volcano scheduler so that's a bit of a moot point regardless.

Removing username seems fine, not a big deal, if users want it they can override. If we remove session name entirely might be a bit of a pain point for tracking.

@d4l3k d4l3k added the RFC Request for Feedback & Roadmaps label Oct 7, 2021
aivanou added a commit to aivanou/torchx-1 that referenced this issue Oct 11, 2021
Summary:
Pull Request resolved: pytorch#215

The diff makes a default torchx session name as `torchx` instead of `torchx-$username`

RFC: pytorch#227

Differential Revision: D31348341

fbshipit-source-id: 1a204a2766cd5def128547cd32ad07b3f6a23ae6
aivanou added a commit to aivanou/torchx-1 that referenced this issue Oct 11, 2021
Summary:
Pull Request resolved: pytorch#215

The diff makes a default torchx session name as `torchx` instead of `torchx-$username`

RFC: pytorch#227

Differential Revision: D31348341

fbshipit-source-id: b1a8c18467cc51ecdb886342088ecb626a94ffa6
aivanou added a commit to aivanou/torchx-1 that referenced this issue Oct 11, 2021
Summary:
Pull Request resolved: pytorch#215

The diff makes a default torchx session name as `torchx` instead of `torchx-$username`

RFC: pytorch#227

Reviewed By: d4l3k

Differential Revision: D31348341

fbshipit-source-id: 9c9f759a34420ddc4b7ef211b477b98057fed7fe
aivanou added a commit to aivanou/torchx-1 that referenced this issue Oct 11, 2021
Summary:
Pull Request resolved: pytorch#215

The diff makes a default torchx session name as `torchx` instead of `torchx-$username`

RFC: pytorch#227

Reviewed By: d4l3k

Differential Revision: D31348341

fbshipit-source-id: dc164139bc693378cf4f15b93f6e4c0324de1fce
facebook-github-bot pushed a commit that referenced this issue Oct 11, 2021
Summary:
Pull Request resolved: #215

The diff makes a default torchx session name as `torchx` instead of `torchx-$username`

RFC: #227

Reviewed By: d4l3k

Differential Revision: D31348341

fbshipit-source-id: ff50599a629907d888cebccdeab948df2a17938d
@aivanou aivanou changed the title Torchx Session Name [torchx][api] Deprecate Torchx Session Name Nov 2, 2021
@kiukchung kiukchung added this to the 0.1.2 release milestone Jan 27, 2022
@kurman
Copy link
Contributor

kurman commented May 12, 2022

torchx run --scheduler local_cwd --scheduler_args log_dir=/tmp utils.echo --msg "Hello $USER" yields:

...
local_cwd://torchx/echo-mvnppz5dsrkzmc
torchx 2022-05-12 12:51:55 INFO Waiting for the app to finish...
torchx 2022-05-12 12:51:55 INFO Job finished: SUCCEEDED

I believe it was closed in #215

@kiukchung
Copy link
Collaborator

lets just add a deprecation warning + docs

@kurman kurman self-assigned this Jun 2, 2022
@kurman kurman closed this as completed Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module: runner issues related to the torchx.runner and torchx.scheduler modules RFC Request for Feedback & Roadmaps
Projects
None yet
Development

No branches or pull requests

4 participants