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

Implement job run command #652

Merged
merged 25 commits into from
Apr 22, 2019
Merged

Implement job run command #652

merged 25 commits into from
Apr 22, 2019

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Mar 20, 2019

neuro run ..., as discussed many times earlier.

@asvetlov asvetlov requested review from shagren and atemate March 20, 2019 15:53
@shagren
Copy link
Contributor

shagren commented Mar 20, 2019

why new command 'run' instead integrating new option --preset to submit/train ?

@@ -458,6 +484,169 @@ def format_fail(job: str, reason: Exception) -> str:
click.echo(format_fail(job, error))


@command(context_settings=dict(ignore_unknown_options=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we use ignore_unknown_options temporarily here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A copy from submit command.
Otherwise executed command (sleep 100 for example) should be quoted.
I know the #526 problem but please let's eat an elephant a bite at a time.

captured = helper.run_cli(["job", "kill", job_id])

# Currently we check that the job is not running anymore
# TODO(adavydow): replace to succeeded check when racecon in
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the comment is redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. I'd like to figure it out (for job submit as well) in a separate PR

python/neuromation/cli/job.py Outdated Show resolved Hide resolved
python/neuromation/cli/job.py Outdated Show resolved Hide resolved
python/neuromation/cli/job.py Outdated Show resolved Hide resolved
python/tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
python/tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
tests/e2e/test_e2e_jobs.py Outdated Show resolved Hide resolved
@neuro-inc neuro-inc deleted a comment from codecov bot Apr 3, 2019
@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #652 into master will increase coverage by 0.83%.
The diff coverage is 79.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #652      +/-   ##
==========================================
+ Coverage   90.71%   91.54%   +0.83%     
==========================================
  Files          40       39       -1     
  Lines        3208     3230      +22     
  Branches      454      440      -14     
==========================================
+ Hits         2910     2957      +47     
+ Misses        220      205      -15     
+ Partials       78       68      -10
Impacted Files Coverage Δ
neuromation/client/jobs.py 93.2% <ø> (ø)
neuromation/cli/main.py 60.1% <100%> (-0.75%) ⬇️
neuromation/cli/job.py 87.85% <79.41%> (+4.52%) ⬆️
neuromation/cli/share.py 84% <0%> (-6%) ⬇️
neuromation/cli/utils.py 91.51% <0%> (-3.49%) ⬇️
neuromation/cli/storage.py 77.08% <0%> (-1.57%) ⬇️
neuromation/cli/version_utils.py 93.22% <0%> (-0.33%) ⬇️
neuromation/cli/defaults.py 100% <0%> (ø) ⬆️
neuromation/cli/const.py 100% <0%> (ø) ⬆️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23daed8...8361cf9. Read the comment docs.

help="Wait for a job start or failure",
)
@async_cmd
async def run(
Copy link
Contributor

Choose a reason for hiding this comment

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

Neuro run should be effectively an alias to Neuro submit, but allow specifying a preset instead of specific resources. Thus, we still want to be able to pass volumes etc. Also, we would like to mount the default volumes only when specified explicitly using an option such as —home or similar.

@astaff @truskovskiyk

README.md Show resolved Hide resolved
README.md Outdated
# sets PYTHONPATH environment value to /python.
# Directories as mounted as in previous example.
# Please note that SSH server should be provided by container.
neuro run --env PYTHONPATH=/python --ssh 22 pytorch:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't --ssh obsolete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

great job!

@asvetlov asvetlov merged commit 0c08550 into master Apr 22, 2019
@asvetlov asvetlov deleted the job-run branch April 22, 2019 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants