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

[Project] Implementing Project CLI #5397

Merged
merged 11 commits into from
Aug 9, 2019
Merged

Conversation

simon-mo
Copy link
Contributor

@simon-mo simon-mo commented Aug 7, 2019

What do these changes do?

Related issue number

Linter

  • I've run scripts/format.sh to lint the changes in this PR.

print(proj)
except (jsonschema.exceptions.ValidationError, ValueError) as e:
print("💔 Validation failed for the following reason", file=sys.stderr)
raise e
Copy link
Contributor

Choose a reason for hiding this comment

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

try raising click exceptions; you won't see the stack trace

@simon-mo simon-mo requested a review from pcmoritz August 7, 2019 22:59
PROJECT_DIR = ".rayproject"
PROJECT_YAML = os.path.join(PROJECT_DIR, "project.yaml")
CLUSTER_YAML = os.path.join(PROJECT_DIR, "cluster.yaml")
REQ_TXT = os.path.join(PROJECT_DIR, "requirements.txt")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with REQUIREMENTS_TXT here to not be too cryptic :)

import logging
import os
import sys
from shutil import copyfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this available by default? Are we using it anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The following files use it:

python/setup.py
rllib/tests/test_checkpoint_restore.py
rllib/tests/test_io.py
python/ray/projects/scripts.py
python/ray/tests/test_basic.py
python/ray/tests/test_autoscaler.py
python/ray/tests/test_tempfile.py
python/ray/tune/trainable.py
python/ray/tune/tests/test_tune_restore.py
python/ray/tune/tests/test_trial_scheduler.py
python/ray/tune/tests/test_experiment_analysis.py
python/ray/tune/tests/test_trial_runner.py
python/ray/tune/tests/test_cluster.py
python/ray/tune/schedulers/pbt.py

I think we can safely assume it exists

"name",
"cluster"
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have trailing newlines (also in the other files, maybe you can configure your editor to add it)

# requirements.txt
_THIS_FILE_DIR = os.path.split(os.path.abspath(__file__))[0]
_TEMPLATE_DIR = os.path.join(_THIS_FILE_DIR, "templates")
PROJECT_TMPL = os.path.join(_TEMPLATE_DIR, "project_template.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with PROJECT_TEMPLATE here

@@ -43,6 +46,12 @@
"ray/autoscaler/local/example-full.yaml",
]

ray_project_files = [
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

# dockerimage: The docker image to be used to run the project in. Like ubuntu:18.04
requirements: {{requirements}}

shell: # shell commands to be ran for environment setup.
Copy link
Contributor

Choose a reason for hiding this comment

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

shell -> # Shell

- echo "Setting up the environment"

commands:
- name: first command
Copy link
Contributor

Choose a reason for hiding this comment

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

command names shouldn't include spaces (replace with first-command?)

"--verbose", help="If set, print the validated file", is_flag=True)
def validate(verbose):
try:
proj = ray.projects.load_project(os.getcwd())
Copy link
Contributor

Choose a reason for hiding this comment

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

proj -> project

requirements = REQ_TXT

with open(PROJECT_TMPL) as f:
proj_tmpl = f.read()
Copy link
Contributor

Choose a reason for hiding this comment

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

proj_tmpl -> project_template

Copy link
Contributor

Choose a reason for hiding this comment

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

agree here - in general, would be good to be more verbose

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@simon-mo simon-mo marked this pull request as ready for review August 8, 2019 19:16
cluster_yaml = CLUSTER_YAML

if requirements is None:
logging.warn("Using default requirements.txt")
Copy link
Contributor

Choose a reason for hiding this comment

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

logger = logging.getLogger?

REQ_TXT_TMPL = os.path.join(_TEMPLATE_DIR, "requirements.txt")


@click.group("project", help="Commands working with ray project")
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs a better help string. @pcmoritz?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, can you mark this explicitly as experimental?

@@ -0,0 +1,101 @@
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

py2to3

Copy link
Contributor

@richardliaw richardliaw left a comment

Choose a reason for hiding this comment

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

Can you add a page to the docs for this marking it as experimental?

@simon-mo
Copy link
Contributor Author

simon-mo commented Aug 8, 2019

@richardliaw I believe @pcmoritz is adding docs in following up PR.

Experimental is marked here:

Usage: ray [OPTIONS] COMMAND [ARGS]...

Options:
  --logging-level TEXT   The logging level threshold, choices=['debug',
                         'info', 'warning', 'error', 'critical'],
                         default='info'
  --logging-format TEXT  The logging format. default='%(asctime)s
                         %(levelname)s %(filename)s:%(lineno)s -- %(message)s'
  --help                 Show this message and exit.

Commands:
...
  project           [Experimental] Commands working with ray project
  rsync-down
  rsync-up
  rsync_down
  rsync_up
  session           [Experimental] Commands working with ray session
...

@simon-mo simon-mo requested a review from pcmoritz August 8, 2019 20:05
@simon-mo simon-mo changed the title [WIP] Implementing Project CLI [Project] Implementing Project CLI Aug 8, 2019
@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

@pcmoritz pcmoritz merged commit d9b45cc into ray-project:master Aug 9, 2019
@AmplabJenkins
Copy link

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

@AmplabJenkins
Copy link

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

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