Add job run-time limit flag to `neuro run` #1325

atemate · 2020-02-11T14:51:42Z

Support run-time limit enforcement functionality: neuro-inc/platform-api#1044

neuromation/api/utils.py

neuromation/cli/job.py

serhiy-storchaka · 2020-02-11T15:27:59Z

You can use the timeout command.

atemate · 2020-02-11T16:43:53Z

You can use the timeout command.

@serhiy-storchaka sorry I didn't understand you: for what?

serhiy-storchaka · 2020-02-11T17:21:53Z

I meant that the user can use the timeout command instead of neuro option to limit the job runtime.

$ neuro run timeout 1h job-command

If add such feature in neuro, I think it should time suffixes m for minutes and h for hours as in admin add-user-quota. It is also compatible with timeout.

serhiy-storchaka · 2020-02-11T17:25:20Z

I think --limit is not good name. We can add other limits in future.

asvetlov

Please update ./docs/jobs_reference.rts at least as well.

neuromation/api/utils.py

neuromation/api/config.py

atemate · 2020-02-19T19:22:30Z

@asvetlov, a few questions if you don't mind:

Any objections from using pytimeparse? It provides the parser for free format time-delta definition like 2d 5h 34m or 2 days, 5 hours, 34 minutes or 5hr34m56s. However, it's officially supported only up to Pyhton 3.6. If we refuse to use it, I'll write similar but simplified regex-based parser to parse strings like 2d 5h 34m, not a problem.
If we change the name of the flag to neuro run --timeout, is that a problem that neuro exec --timeout already exists, but --timeout there has slightly different meaning?
How can we (and should we) write the default config value to the user's yml-config? I see method calc_columns addresses the default value COLUMNS if no value found in config, so here we repeat this algorithm for method calc_default_job_timeout. However, I thought it might be useful if we write this default value to the yml-config, and just let the user modify it -- it's easier.

asvetlov · 2020-02-20T10:14:47Z

@asvetlov, a few questions if you don't mind:

Any objections from using pytimeparse? It provides the parser for free format time-delta definition like 2d 5h 34m or 2 days, 5 hours, 34 minutes or 5hr34m56s. However, it's officially supported only up to Pyhton 3.6. If we refuse to use it, I'll write similar but simplified regex-based parser to parse strings like 2d 5h 34m, not a problem.

A free format is not good for usage in CLI parameters: 2d 5h 34m should be enclosed in brackets, 2 days, 5 hours, 34 minutes is even worse because of its length.

If we change the name of the flag to neuro run --timeout, is that a problem that neuro exec --timeout already exists, but --timeout there has slightly different meaning?

Please use --life-span, the term is very well known, short enough and unambiguous.

How can we (and should we) write the default config value to the user's yml-config? I see method calc_columns addresses the default value COLUMNS if no value found in config, so here we repeat this algorithm for method calc_default_job_timeout. However, I thought it might be useful if we write this default value to the yml-config, and just let the user modify it -- it's easier.

The philosophy of user-provided configuration files is that they are never modified by the application itself. Even a command for user config file changing is redundant -- every software engineer knows how to use a text editor. Please use the hard-coded default value if absent.

P.S. I wonder why do you use yml-config term for toml file format?

atemate · 2020-02-20T12:18:24Z

Re-worked this PR considering the comments.

Not a big deal not to use a side library, the hand-written parser turned out to be small and supporting more concise format: 1d 2h 3m 4s or 1d2h3m4s or 1d 3m or 1d or 0 (with or without spaces, where some parts may be missing, or 0 as an edge-case since 0s is the same as 0m).
I never heard of --life-span in the sense of timeout, but it does not matter for me, I did the --life-span.
I would think of a user-config as of a normal Unix config (a .conf file somewhere inside /etc/), which contains all sections with defaults in the right format but commented out. But I agree that, comparing to Unix utilities, neuro is evolving fastly, and the format of the user-config is changed fastly either. So this is apparently the part of documentation, to teach user the format.
BTW, note the format of the new section:

[job.default-life-span]
days = 1
hours = 2
minutes = 3

P.S. I wonder why do you use yml-config term for toml file format?

my bad, a typo-like mistake.

atemate · 2020-02-20T12:26:42Z

New question: What should be the short option for --life-time? Should it be at all?

atemate · 2020-02-20T12:32:22Z

neuromation/cli/job.py

+    elif seconds < 0:
+        raise click.UsageError(f"Job life span must be non-negative, got: '{result}'")


technically, our input parser does not allow negative values, so they can come only from the config. I understand, it's better to check it during user config validation, but it's quicker to do it here, after parsing.

Should I add more validation code to _validate_user_config(), similarly as it is with _validate_alias()?

Why don't we use trafaret? Doesn't it have enough expressive power to validate aliases, does it?

technically, our input parser does not allow negative values, so they can come only from the config. I understand, it's better to check it during user config validation, but it's quicker to do it here, after parsing.

Should I add more validation code to _validate_user_config(), similarly as it is with _validate_alias()?

Please keep as is until we implement something like neuro config validate command.

Why don't we use trafaret? Doesn't it have enough expressive power to validate aliases, does it?

Trafaret provides machine-readable error messages which should be translated to human-readable texts before printing. It makes trafaret virtually useless for our needs.

Why not to use extract_error? It might be easier to use a package that was designed to write custom validators, rather than to implement our own from scratch.

asvetlov · 2020-02-20T13:07:29Z

New question: What should be the short option for --life-time? Should it be at all?

If you are in doubt -- don't use short name, that's fine.

neuromation/api/config.py

neuromation/api/jobs.py

neuromation/cli/job.py

asvetlov · 2020-02-20T13:19:53Z

neuromation/cli/job.py

@@ -889,6 +927,13 @@ def format_fail(job: str, reason: Exception) -> str:
    if not wait_start:
        detach = True

+    job_life_span = await calc_life_span(root.client, life_span)
+    if not root.quiet:


I rather prefer to see this printout in explicit verbose mode only. In both normal and quiet modes the message should be absent.

Isn't the information that your job will die automatically after certain (in some cases, implicit) amount of time a more important information rather than, say, Shortcuts? I think, if the job has a timeout, it should be printed in non-quiet mode. Other question is, perhaps, a better place for it is formatters/jobs.py?

There is a lot of important information about the job. We need a balance between insufficient and too verbose outputs.
Moving reports about remaining time is a very cool idea.
I see the REST API inconsistency here though: the deadline timestamp is not returned in JobDescription data structure. Seems like the server doesn't file this info, that's sad. In my mind, it should be an absolute UNIX timestamp; relative times are not reliable. The error is one less significant digit due to network latency, 1 minute in your case.
It raises another question: should the absolute timestamp be used as input in REST API? Passing a relative time in Python API is ok.

asvetlov · 2020-02-20T13:23:59Z

neuromation/cli/job.py

+    elif seconds < 0:
+        raise click.UsageError(f"Job life span must be non-negative, got: '{result}'")


technically, our input parser does not allow negative values, so they can come only from the config. I understand, it's better to check it during user config validation, but it's quicker to do it here, after parsing.

Should I add more validation code to _validate_user_config(), similarly as it is with _validate_alias()?

Please keep as is until we implement something like neuro config validate command.

Why don't we use trafaret? Doesn't it have enough expressive power to validate aliases, does it?

Trafaret provides machine-readable error messages which should be translated to human-readable texts before printing. It makes trafaret virtually useless for our needs.

neuromation/cli/job.py

asvetlov

Please document

neuromation/cli/job.py

neuromation/api/jobs.py

asvetlov · 2020-02-21T15:57:51Z

docs/jobs_reference.rst

@@ -134,6 +135,9 @@ Jobs
                                     cannot be scheduled because the lack of computation
                                     cluster resources (memory, CPU/GPU etc).

+      :param float life_span: job run-time limit in the format '1d2h3m4s'


The '1d2h3m4s' is invalid float.
What is None for? I assume it is for server default, please correct me if I'm wrong.

None to disable. I'd actually leave the default API value for 1d, not for None as it is now.

Why not enforce the default on server side?
It is more reliable.
I feel potential problems if we want to change 1day to another default eventually.

Long time ago, when I was implementing this feature on the backend, my very first version enforced default server-side as you suggest. However, after several review we agreed to pass default value by the client so that:

no problems with DB migration, or if we decide not to write the default value to the database, then no implicit logic;

much less code

not over-complicated protocol (0 means limit=0)

easier to change the default value per-user: no need to put it to the database.

docs/jobs_reference.rst

asvetlov

Last neats.

asvetlov · 2020-02-25T13:52:55Z

neuromation/cli/job.py

+    return seconds
+
+
+async def calc_default_life_span(client: Client) -> timedelta:


Inline the function in calc_life_span() please.

Why? it's harder to test

The function is never used alone, it is always called from calc_life_span().
You can test calc_life_span(client, None) instead of calc_default_life_span(client), should prodice the same result.

the matter of taste. changed.

asvetlov · 2020-02-25T13:53:20Z

neuromation/cli/job.py

+    seconds = delta.total_seconds()
+    if seconds == 0:
+        return None
+    assert seconds > 0, f"life-span cannot be negative, got: {seconds}"


Please raise ValueError instead of AssertionError here.

It's impossible situation, and we need this check only in case in order to protect ourselves from sending negative values to the server one day if we change something in future (for example, we are going to change toml parsers). So I cannot test this check properly. Here's what we can do:

leave assertion

change to ValueError but still keep it untested

remove the check completely

I read assert expr, msg as something that you want to expose to a user.
If the situation is impossible I expect bare assert expr at least. Like for the sake of mypy correctness, we have assert var is not None very often without a custom provided error message.

I was always thinking that assert expr is a bad practice since if anything ever goes wrong, the user will not get any idea on the reason, just AssertionError.

changed to bare assert expr

atemate commented Feb 11, 2020

View reviewed changes

neuromation/api/utils.py Outdated Show resolved Hide resolved

atemate commented Feb 11, 2020

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

atemate marked this pull request as ready for review February 11, 2020 15:05

asvetlov requested changes Feb 12, 2020

View reviewed changes

neuromation/api/utils.py Outdated Show resolved Hide resolved

asvetlov reviewed Feb 12, 2020

View reviewed changes

neuromation/api/config.py Outdated Show resolved Hide resolved

Artem Yushkovskiy added 2 commits February 19, 2020 18:21

Save default limit to local config

c606997

free format

4427f5b

Artem Yushkovskiy added 2 commits February 20, 2020 14:34

custom parser

d57e9cc

complete

e59abf7

atemate force-pushed the ay/lifetime branch from 914a3ee to e59abf7 Compare February 20, 2020 12:07

Artem Yushkovskiy added 2 commits February 20, 2020 15:19

rename,chlog

8dde0ab

cleanup

643b41a

atemate commented Feb 20, 2020

View reviewed changes

asvetlov reviewed Feb 20, 2020

View reviewed changes

Artem Yushkovskiy added 3 commits February 20, 2020 20:48

review

7931f4c

cleanup

5388319

Merge branch 'master' into ay/lifetime

52701cd

asvetlov reviewed Feb 21, 2020

View reviewed changes

neuromation/cli/job.py Show resolved Hide resolved

asvetlov requested changes Feb 21, 2020

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

neuromation/cli/job.py Outdated Show resolved Hide resolved

neuromation/api/jobs.py Show resolved Hide resolved

Artem Yushkovskiy added 2 commits February 21, 2020 18:49

review-final

e1c421a

cleanup changeslog

59ba7f8

asvetlov reviewed Feb 21, 2020

View reviewed changes

finale

8526cb9

atemate requested a review from asvetlov February 21, 2020 17:26

asvetlov requested changes Feb 25, 2020

View reviewed changes

Artem Yushkovskiy added 2 commits February 25, 2020 18:36

inline

c14913a

assert

6782f93

asvetlov approved these changes Feb 25, 2020

View reviewed changes

atemate merged commit 7808497 into master Feb 26, 2020

atemate deleted the ay/lifetime branch February 26, 2020 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add job run-time limit flag to `neuro run` #1325

Add job run-time limit flag to `neuro run` #1325

atemate commented Feb 11, 2020

serhiy-storchaka commented Feb 11, 2020

atemate commented Feb 11, 2020

serhiy-storchaka commented Feb 11, 2020

serhiy-storchaka commented Feb 11, 2020

asvetlov left a comment

atemate commented Feb 19, 2020

asvetlov commented Feb 20, 2020

atemate commented Feb 20, 2020 •

edited

Loading

atemate commented Feb 20, 2020

atemate Feb 20, 2020

asvetlov Feb 20, 2020

atemate Feb 25, 2020

asvetlov commented Feb 20, 2020

asvetlov Feb 20, 2020

atemate Feb 20, 2020

asvetlov Feb 20, 2020

asvetlov Feb 20, 2020

asvetlov left a comment

asvetlov Feb 21, 2020

atemate Feb 21, 2020

atemate Feb 21, 2020

asvetlov Feb 21, 2020

atemate Feb 25, 2020

asvetlov left a comment

asvetlov Feb 25, 2020

atemate Feb 25, 2020

asvetlov Feb 25, 2020

atemate Feb 25, 2020

asvetlov Feb 25, 2020

atemate Feb 25, 2020

asvetlov Feb 25, 2020

atemate Feb 25, 2020

atemate Feb 25, 2020

		elif seconds < 0:
		raise click.UsageError(f"Job life span must be non-negative, got: '{result}'")

		return seconds


		async def calc_default_life_span(client: Client) -> timedelta:

Add job run-time limit flag to neuro run #1325

Add job run-time limit flag to neuro run #1325

Conversation

atemate commented Feb 11, 2020

serhiy-storchaka commented Feb 11, 2020

atemate commented Feb 11, 2020

serhiy-storchaka commented Feb 11, 2020

serhiy-storchaka commented Feb 11, 2020

asvetlov left a comment

Choose a reason for hiding this comment

atemate commented Feb 19, 2020

asvetlov commented Feb 20, 2020

atemate commented Feb 20, 2020 • edited Loading

atemate commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add job run-time limit flag to `neuro run` #1325

Add job run-time limit flag to `neuro run` #1325

atemate commented Feb 20, 2020 •

edited

Loading