Skip to content

[Projects] Add "session execute"#5681

Merged
pcmoritz merged 11 commits intoray-project:masterfrom
pcmoritz:session-execute
Sep 11, 2019
Merged

[Projects] Add "session execute"#5681
pcmoritz merged 11 commits intoray-project:masterfrom
pcmoritz:session-execute

Conversation

@pcmoritz
Copy link
Copy Markdown
Contributor

@pcmoritz pcmoritz commented Sep 10, 2019

Why are these changes needed?

This PR introduces a new session execute command which allows running commands in an already created session.

Related issue number

Checks

raise click.ClickException(
"Docker support in session is currently not implemented. "
"Please file an feature request at"
"https://github.com/ray-project/ray/issues")
Copy link
Copy Markdown
Contributor

@richardliaw richardliaw Sep 10, 2019

Choose a reason for hiding this comment

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

I actually don't like this practice; I don't think it's particularly useful (suggesting the filing, that is).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(though it's not the end of the world if you leave it in)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I removed it

# We try to parse the command here so we can fail before the cluster
# is started in case the command is malformed.
if shell:
self.command_to_run = command
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it actually might make more sense to pass in the command to run to SessionRunner.execute_command instead of the constructor. This would also let us reuse the same SessionRunner for multiple commands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea!

def session_start(command, args, shell):
runner = SessionRunner(command, args, shell)
if shell or command:
logger.info("[1/4] Creating cluster")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To reduce duplication here, maybe just set the total number of steps in the if statement?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@AmplabJenkins
Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16949/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16947/
Test FAILed.

@AmplabJenkins
Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16950/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16951/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16957/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16961/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16962/
Test FAILed.

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16965/
Test FAILed.

@pcmoritz pcmoritz merged commit 9ce6dd9 into ray-project:master Sep 11, 2019
@pcmoritz pcmoritz deleted the session-execute branch September 11, 2019 07:50
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.

4 participants