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

add timeout for validators #207

Merged
merged 6 commits into from
May 14, 2022
Merged

add timeout for validators #207

merged 6 commits into from
May 14, 2022

Conversation

mzuenni
Copy link
Collaborator

@mzuenni mzuenni commented May 10, 2022

I am not sure if this breaks something else...

bin/program.py Show resolved Hide resolved
bin/validate.py Outdated Show resolved Hide resolved
bin/program.py Show resolved Hide resolved
bin/config.py Outdated
@@ -74,7 +78,7 @@
grep -Ev '^(h|jobs|time|verbose)$' | sed "s/^/'/;s/$/',/" | tr '\n' ' ' | sed 's/^/args_list = [/;s/, $/]\n/'

Choose a reason for hiding this comment

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

Add timeout to this list

@@ -37,7 +37,7 @@ def run_interactive_testcase(
output_validator = output_validators[0]

# Set limits
validator_timeout = 60
validator_timeout = 2*config.DEFAULT_TIMEOUT

Choose a reason for hiding this comment

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

Better make this a separate constant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@RagnarGrootKoerkamp RagnarGrootKoerkamp merged commit 2cabb79 into RagnarGrootKoerkamp:master May 14, 2022
@RagnarGrootKoerkamp
Copy link
Owner

RagnarGrootKoerkamp commented May 14, 2022

Thanks!

@mzuenni mzuenni deleted the validator_timeout branch May 15, 2022 20:36
@mpsijm
Copy link
Collaborator

mpsijm commented May 17, 2022

@mzuenni With bt run, the abort timeout for TLE submissions now seems to be always 30 seconds, instead of 1.5 * time_limit. Is that intentional?

@@ -64,17 +68,18 @@
'jobs': os.cpu_count() // 2,
'time': 600, # Used for `bt fuzz`
'verbose': 0,
'timeout': DEFAULT_TIMEOUT,

Choose a reason for hiding this comment

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

The new issue is probably caused by this, since the computation for 1.5*timelimit now thinks there is a timelimit set

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, in that case, it is probably a bad idea to set the timeout with a default value. I could instead add a function like this:

def get_timeout(default=DEFAULT_TIMEOUT):
    return args.timeout or default

or i could add the or DEFAULT_TIMEOUT everywhere. I am not sure which is better practice.

Choose a reason for hiding this comment

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

Either is fine

Choose a reason for hiding this comment

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

Alternatively, you could explicitly check if the timeout argument is set for solution timeout computation

@mzuenni
Copy link
Collaborator Author

mzuenni commented May 17, 2022

That was not intentional and is somewhat related to #165? I was not aware that timeout is used for the TLE submissions ):

the question is, do we want to keep the current behavior (and remove the default timeout as described above) or always set the problem TLE timeout to 1.5*timelimit+1 (and maybe even add the safety_margin option to change the 1.5 constant)?

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.

3 participants